From 211f112d5568c2714c4a6e10e72dba21143317c4 Mon Sep 17 00:00:00 2001 From: Evan Hahn Date: Mon, 28 Sep 2026 18:23:20 +0200 Subject: [PATCH 01/21] TypeScriptified limits service (#31032) no ref This change should have no user impact. --- .../core/core/server/api/endpoints/themes.js | 2 +- .../endpoints/utils/validators/input/files.js | 2 +- .../endpoints/utils/validators/input/media.js | 2 +- .../importer/importers/data/users-importer.js | 2 +- ghost/core/core/server/models/integration.js | 2 +- ghost/core/core/server/models/invite.js | 2 +- ghost/core/core/server/models/post.js | 2 +- ghost/core/core/server/models/user.js | 2 +- .../server/services/auth/api-key/admin.js | 2 +- .../server/services/auth/api-key/content.js | 2 +- .../email-service/email-service-wrapper.js | 2 +- .../server/services/{limits.js => limits.ts} | 28 +++++++++---------- .../core/server/services/newsletters/index.js | 2 +- .../server/services/settings-helpers/index.js | 2 +- .../services/settings/settings-service.js | 2 +- .../core/server/services/themes/installer.js | 2 +- .../core/server/services/webhooks/index.js | 2 +- .../services/webhooks/webhook-trigger.js | 2 +- .../test/e2e-api/admin/host-limits.test.ts | 10 ++----- .../core/test/e2e-api/admin/settings.test.js | 4 +-- .../test/unit/server/services/limits.test.js | 11 +++++--- .../settings/settings-service.test.js | 2 +- .../test/utils/e2e-framework-mock-manager.js | 2 +- ghost/core/test/utils/host-limits-utils.ts | 10 ++----- 24 files changed, 47 insertions(+), 54 deletions(-) rename ghost/core/core/server/services/{limits.js => limits.ts} (69%) diff --git a/ghost/core/core/server/api/endpoints/themes.js b/ghost/core/core/server/api/endpoints/themes.js index dc1ba2b3e75..7ca282a338b 100644 --- a/ghost/core/core/server/api/endpoints/themes.js +++ b/ghost/core/core/server/api/endpoints/themes.js @@ -1,5 +1,5 @@ const themeService = require('../../services/themes'); -const limitService = require('../../services/limits'); +const { limitService } = require('../../services/limits'); const models = require('../../models'); // Used to emit theme.uploaded which is used in core/server/analytics-events diff --git a/ghost/core/core/server/api/endpoints/utils/validators/input/files.js b/ghost/core/core/server/api/endpoints/utils/validators/input/files.js index a65e41f60c2..c6817dce24d 100644 --- a/ghost/core/core/server/api/endpoints/utils/validators/input/files.js +++ b/ghost/core/core/server/api/endpoints/utils/validators/input/files.js @@ -1,4 +1,4 @@ -const limitService = require('../../../../../services/limits'); +const { limitService } = require('../../../../../services/limits'); module.exports = { async upload(apiConfig, frame) { diff --git a/ghost/core/core/server/api/endpoints/utils/validators/input/media.js b/ghost/core/core/server/api/endpoints/utils/validators/input/media.js index e975cd5cf01..23485844b37 100644 --- a/ghost/core/core/server/api/endpoints/utils/validators/input/media.js +++ b/ghost/core/core/server/api/endpoints/utils/validators/input/media.js @@ -1,4 +1,4 @@ -const limitService = require('../../../../../services/limits'); +const { limitService } = require('../../../../../services/limits'); module.exports = { async upload(apiConfig, frame) { diff --git a/ghost/core/core/server/data/importer/importers/data/users-importer.js b/ghost/core/core/server/data/importer/importers/data/users-importer.js index 1745c6d6693..91dfa6e3563 100644 --- a/ghost/core/core/server/data/importer/importers/data/users-importer.js +++ b/ghost/core/core/server/data/importer/importers/data/users-importer.js @@ -2,7 +2,7 @@ const debug = require('@tryghost/debug')('importer:users'); const _ = require('lodash'); const BaseImporter = require('./base'); const models = require('../../../../models'); -const limitService = require('../../../../services/limits'); +const { limitService } = require('../../../../services/limits'); class UsersImporter extends BaseImporter { constructor(allDataFromFile) { diff --git a/ghost/core/core/server/models/integration.js b/ghost/core/core/server/models/integration.js index 752b95bc741..5db07804574 100644 --- a/ghost/core/core/server/models/integration.js +++ b/ghost/core/core/server/models/integration.js @@ -1,5 +1,5 @@ const _ = require('lodash'); -const limitService = require('../services/limits'); +const { limitService } = require('../services/limits'); const ghostBookshelf = require('./base'); const errors = require('@tryghost/errors'); const { NoPermissionError } = errors; diff --git a/ghost/core/core/server/models/invite.js b/ghost/core/core/server/models/invite.js index 7b56f5f1ede..e2d6c1de408 100644 --- a/ghost/core/core/server/models/invite.js +++ b/ghost/core/core/server/models/invite.js @@ -4,7 +4,7 @@ const security = require('@tryghost/security'); const moment = require('moment'); const settingsCache = require('../../shared/settings-cache'); -const limitService = require('../services/limits'); +const { limitService } = require('../services/limits'); const ghostBookshelf = require('./base'); const { setIsRoles } = require('./role-utils'); diff --git a/ghost/core/core/server/models/post.js b/ghost/core/core/server/models/post.js index bbd7688d545..63dee41aad3 100644 --- a/ghost/core/core/server/models/post.js +++ b/ghost/core/core/server/models/post.js @@ -9,7 +9,7 @@ const htmlToPlaintext = require('@tryghost/html-to-plaintext'); const ghostBookshelf = require('./base'); const config = require('../../shared/config'); const settingsCache = require('../../shared/settings-cache'); -const limitService = require('../services/limits'); +const { limitService } = require('../services/limits'); const mobiledocLib = require('../lib/mobiledoc'); const lexicalLib = require('../lib/lexical'); const relations = require('./relations'); diff --git a/ghost/core/core/server/models/user.js b/ghost/core/core/server/models/user.js index 0fce6a2c214..0617265b5c0 100644 --- a/ghost/core/core/server/models/user.js +++ b/ghost/core/core/server/models/user.js @@ -2,7 +2,7 @@ const validator = require('@tryghost/validator'); const ObjectId = require('bson-objectid').default; const ghostBookshelf = require('./base'); const baseUtils = require('./base/utils'); -const limitService = require('../services/limits'); +const { limitService } = require('../services/limits'); const tpl = require('@tryghost/tpl'); const errors = require('@tryghost/errors'); const security = require('@tryghost/security'); diff --git a/ghost/core/core/server/services/auth/api-key/admin.js b/ghost/core/core/server/services/auth/api-key/admin.js index 014a57bd254..f45132bac9a 100644 --- a/ghost/core/core/server/services/auth/api-key/admin.js +++ b/ghost/core/core/server/services/auth/api-key/admin.js @@ -2,7 +2,7 @@ const jwt = require('jsonwebtoken'); const url = require('url'); const models = require('../../../models'); const errors = require('@tryghost/errors'); -const limitService = require('../../../services/limits'); +const { limitService } = require('../../../services/limits'); const { legacyApiPathMatch } = require('../../../web/api/middleware/api-version-compatibility'); const tpl = require('@tryghost/tpl'); const _ = require('lodash'); diff --git a/ghost/core/core/server/services/auth/api-key/content.js b/ghost/core/core/server/services/auth/api-key/content.js index feba1f9c02f..1d89e782ca0 100644 --- a/ghost/core/core/server/services/auth/api-key/content.js +++ b/ghost/core/core/server/services/auth/api-key/content.js @@ -1,6 +1,6 @@ const models = require('../../../models'); const errors = require('@tryghost/errors'); -const limitService = require('../../../services/limits'); +const { limitService } = require('../../../services/limits'); const tpl = require('@tryghost/tpl'); const messages = { diff --git a/ghost/core/core/server/services/email-service/email-service-wrapper.js b/ghost/core/core/server/services/email-service/email-service-wrapper.js index 53ea3a65062..58a2392b5fb 100644 --- a/ghost/core/core/server/services/email-service/email-service-wrapper.js +++ b/ghost/core/core/server/services/email-service/email-service-wrapper.js @@ -42,7 +42,7 @@ class EmailServiceWrapper { const db = require('../../data/db'); const sentry = require('../../../shared/sentry'); const membersRepository = membersService.api.members; - const limitService = require('../limits'); + const { limitService } = require('../limits'); const labs = require('../../../shared/labs'); const emailAddressService = require('../email-address'); const i18nLib = require('@tryghost/i18n').default; diff --git a/ghost/core/core/server/services/limits.js b/ghost/core/core/server/services/limits.ts similarity index 69% rename from ghost/core/core/server/services/limits.js rename to ghost/core/core/server/services/limits.ts index ddd8c25c7ce..87fbfae55ba 100644 --- a/ghost/core/core/server/services/limits.js +++ b/ghost/core/core/server/services/limits.ts @@ -1,11 +1,14 @@ -const errors = require('@tryghost/errors'); -const config = require('../../shared/config'); -const db = require('../data/db'); -const logging = require('@tryghost/logging'); -const { LimitService } = require('@tryghost/limit-service'); -const limitService = new LimitService(); +import errors from '@tryghost/errors'; +import logging from '@tryghost/logging'; +import { LimitService } from '@tryghost/limit-service'; +import type { Subscription } from '@tryghost/limit-service'; +import type { RequestHandler } from 'express'; +import config from '../../shared/config'; +import db from '../data/db'; -const init = () => { +export const limitService = new LimitService(); + +export const init = () => { let helpLink; if ( @@ -18,7 +21,7 @@ const init = () => { helpLink = 'https://ghost.org/help/'; } - let subscription; + let subscription: Subscription | undefined; if (config.get('hostSettings:subscription')) { subscription = { @@ -52,8 +55,8 @@ const init = () => { * change it. Answers 403 with the host's own wording, which is a different thing to tell a * caller than the 404 a labs flag gives: the feature exists, this plan does not include it. */ -const requireFeature = (limitName) => - async function requireFeatureMw(req, res, next) { +export const requireFeature = (limitName: string): RequestHandler => + async function requireFeatureMw(_req, _res, next) { try { await limitService.errorIfWouldGoOverLimit(limitName); next(); @@ -61,8 +64,3 @@ const requireFeature = (limitName) => next(err); } }; - -module.exports = limitService; - -module.exports.init = init; -module.exports.requireFeature = requireFeature; diff --git a/ghost/core/core/server/services/newsletters/index.js b/ghost/core/core/server/services/newsletters/index.js index c69f1aa1f82..beb877e4282 100644 --- a/ghost/core/core/server/services/newsletters/index.js +++ b/ghost/core/core/server/services/newsletters/index.js @@ -3,7 +3,7 @@ const SingleUseTokenProvider = require('../members/single-use-token-provider'); const mail = require('../mail'); const models = require('../../models'); const urlUtils = require('../../../shared/url-utils').default; -const limitService = require('../limits'); +const { limitService } = require('../limits'); const labs = require('../../../shared/labs'); const emailAddressService = require('../email-address'); diff --git a/ghost/core/core/server/services/settings-helpers/index.js b/ghost/core/core/server/services/settings-helpers/index.js index e9848ea9d51..8a46a6854ce 100644 --- a/ghost/core/core/server/services/settings-helpers/index.js +++ b/ghost/core/core/server/services/settings-helpers/index.js @@ -3,6 +3,6 @@ const urlUtils = require('../../../shared/url-utils').default; const config = require('../../../shared/config'); const SettingsHelpers = require('./settings-helpers'); const labs = require('../../../shared/labs'); -const limitService = require('../limits'); +const { limitService } = require('../limits'); module.exports = new SettingsHelpers({ settingsCache, urlUtils, config, labs, limitService }); diff --git a/ghost/core/core/server/services/settings/settings-service.js b/ghost/core/core/server/services/settings/settings-service.js index b6c9929c67d..6180d247dc1 100644 --- a/ghost/core/core/server/services/settings/settings-service.js +++ b/ghost/core/core/server/services/settings/settings-service.js @@ -5,7 +5,7 @@ const events = require('../../lib/common/events'); const models = require('../../models'); const labs = require('../../../shared/labs'); -const limits = require('../limits'); +const { limitService: limits } = require('../limits'); const config = require('../../../shared/config'); const adapterManager = require('../adapter-manager').default; const SettingsCache = require('../../../shared/settings-cache'); diff --git a/ghost/core/core/server/services/themes/installer.js b/ghost/core/core/server/services/themes/installer.js index 961be3531a8..24162bf60a5 100644 --- a/ghost/core/core/server/services/themes/installer.js +++ b/ghost/core/core/server/services/themes/installer.js @@ -4,7 +4,7 @@ const path = require('path'); const security = require('@tryghost/security'); const request = require('@tryghost/request'); const errors = require('@tryghost/errors'); -const limitService = require('../../services/limits'); +const { limitService } = require('../../services/limits'); const { setFromZip } = require('./storage'); const messages = { diff --git a/ghost/core/core/server/services/webhooks/index.js b/ghost/core/core/server/services/webhooks/index.js index 8389e8c1022..cb43776fbc8 100644 --- a/ghost/core/core/server/services/webhooks/index.js +++ b/ghost/core/core/server/services/webhooks/index.js @@ -5,7 +5,7 @@ module.exports = { listen() { const models = require('../../models'); - const limitService = require('../../services/limits'); + const { limitService } = require('../../services/limits'); const events = require('../../lib/common/events'); const urlService = require('../url'); const createSerialize = require('./serialize'); diff --git a/ghost/core/core/server/services/webhooks/webhook-trigger.js b/ghost/core/core/server/services/webhooks/webhook-trigger.js index 134b7119824..d350c5316dc 100644 --- a/ghost/core/core/server/services/webhooks/webhook-trigger.js +++ b/ghost/core/core/server/services/webhooks/webhook-trigger.js @@ -10,7 +10,7 @@ class WebhookTrigger { * @param {Object} options * @param {Object} options.models - Ghost models * @param {Function} options.payload - Function to generate payload - * @param {import('../../services/limits')} options.limitService - Function to generate payload + * @param {typeof import('../../services/limits').limitService} options.limitService - Limit service * @param {Object} [options.request] - HTTP request handling library */ constructor({ models, payload, request, limitService }) { diff --git a/ghost/core/test/e2e-api/admin/host-limits.test.ts b/ghost/core/test/e2e-api/admin/host-limits.test.ts index dbecc242a90..e4d65c1fd9b 100644 --- a/ghost/core/test/e2e-api/admin/host-limits.test.ts +++ b/ghost/core/test/e2e-api/admin/host-limits.test.ts @@ -22,7 +22,9 @@ const mailService = require('../../../core/server/services/mail') as { const membersService = require('../../../core/server/services/members') as { stripeConnect: StripeConnect; }; -const limits = require('../../../core/server/services/limits') as LimitService; +const { + limitService: limits, +}: typeof import('../../../core/server/services/limits') = require('../../../core/server/services/limits'); /** What an Admin API request answers with, narrowed to the parts these tests read. */ interface ApiResponse { @@ -61,12 +63,6 @@ interface HostLimits { restoreHostLimits(): Promise; } -interface LimitService { - isLimited(name: string): boolean; - isDisabled(name: string): boolean | undefined; - problems: Array<{ limit: string; reason: string }>; -} - interface StripeConnect { getStripeConnectTokenData(): Promise; } diff --git a/ghost/core/test/e2e-api/admin/settings.test.js b/ghost/core/test/e2e-api/admin/settings.test.js index 73c1165b133..e637b93cd63 100644 --- a/ghost/core/test/e2e-api/admin/settings.test.js +++ b/ghost/core/test/e2e-api/admin/settings.test.js @@ -14,7 +14,7 @@ const { const { stringMatching, anyEtag, anyUuid, anyContentLength, anyContentVersion } = matchers; const models = require('../../../core/server/models'); const membersService = require('../../../core/server/services/members'); -const limits = require('../../../core/server/services/limits'); +const { limitService: limits } = require('../../../core/server/services/limits'); const { anyErrorId } = matchers; // Updated to reflect current total based on test output @@ -918,7 +918,7 @@ describe('Settings API', function () { describe('publicSiteAccess limit', function () { function stubPublicSiteAccessDisabled(disabled) { // Stub the singleton directly rather than driving the limit through configUtils + - // limits.init(). The hostSettings.limits config is registered once at boot from the + // service initialization. The hostSettings.limits config is registered once at boot from the // `@tryghost/limit-service` allowlist; bumps of that package are owned by Renovate // so this PR cannot rely on `publicSiteAccess` being a recognised name yet. sinon.stub(limits, 'isDisabled').withArgs('publicSiteAccess').returns(disabled); diff --git a/ghost/core/test/unit/server/services/limits.test.js b/ghost/core/test/unit/server/services/limits.test.js index d77215593dd..a7369bf8fe1 100644 --- a/ghost/core/test/unit/server/services/limits.test.js +++ b/ghost/core/test/unit/server/services/limits.test.js @@ -1,7 +1,10 @@ const assert = require('node:assert/strict'); const sinon = require('sinon'); -const limits = require('../../../../core/server/services/limits'); +const { + limitService: limits, + init: initLimits, +} = require('../../../../core/server/services/limits'); const configUtils = require('../../../utils/config-utils'); const logging = require('@tryghost/logging'); @@ -29,14 +32,14 @@ describe('Limit Service Init', function () { it('initiates and loads limits - minimal setup', async function () { limitServiceStub.returns(Promise.resolve()); - await limits.init(); + await initLimits(); sinon.assert.notCalled(loggerStub.warn); }); it('handles limit-service incorrect usage errors gracefully with a warning', async function () { limitServiceStub.throws(new errors.IncorrectUsageError('Incorrect limits')); - await limits.init(); + await initLimits(); sinon.assert.called(loggerStub.warn); }); @@ -45,7 +48,7 @@ describe('Limit Service Init', function () { limitServiceStub.throws(thrownError); try { - await limits.init(); + await initLimits(); } catch (error) { sinon.assert.notCalled(loggerStub.warn); assert.deepEqual(error, thrownError); diff --git a/ghost/core/test/unit/server/services/settings/settings-service.test.js b/ghost/core/test/unit/server/services/settings/settings-service.test.js index 62a440294e2..a27e401b72d 100644 --- a/ghost/core/test/unit/server/services/settings/settings-service.test.js +++ b/ghost/core/test/unit/server/services/settings/settings-service.test.js @@ -5,7 +5,7 @@ const settingsCache = require('../../../../../core/shared/settings-cache'); const logging = require('@tryghost/logging'); const { Settings } = require('../../../../../core/server/models/settings'); const adapterManager = require('../../../../../core/server/services/adapter-manager').default; -const limits = require('../../../../../core/server/services/limits'); +const { limitService: limits } = require('../../../../../core/server/services/limits'); describe('Settings Service', function () { let settingsService; diff --git a/ghost/core/test/utils/e2e-framework-mock-manager.js b/ghost/core/test/utils/e2e-framework-mock-manager.js index 8a3e91f9a6c..e7617697981 100644 --- a/ghost/core/test/utils/e2e-framework-mock-manager.js +++ b/ghost/core/test/utils/e2e-framework-mock-manager.js @@ -27,7 +27,7 @@ const originalMailServiceSendMail = mailService.GhostMailer.prototype.sendMail; const labs = require('../../core/shared/labs'); const events = require('../../core/server/lib/common/events'); const settingsCache = require('../../core/shared/settings-cache'); -const limitService = require('../../core/server/services/limits'); +const { limitService } = require('../../core/server/services/limits'); const dns = require('dns'); const dnsPromises = dns.promises; const StripeMocker = require('./stripe-mocker'); diff --git a/ghost/core/test/utils/host-limits-utils.ts b/ghost/core/test/utils/host-limits-utils.ts index 44f7104e926..5217101f1b7 100644 --- a/ghost/core/test/utils/host-limits-utils.ts +++ b/ghost/core/test/utils/host-limits-utils.ts @@ -1,16 +1,12 @@ -// Both of these are CommonJS with no types of their own, so the shape this file relies on -// is stated here rather than inferred as `any`. +import * as limits from '../../core/server/services/limits'; + +// Config utils is CommonJS with no types of its own, so state its shape here. interface ConfigUtils { set(config: Record): void; restore(): Promise; } -interface LimitService { - init(): void; -} - const configUtils = require('./config-utils') as ConfigUtils; -const limits = require('../../core/server/services/limits') as LimitService; /** One limit as a host configures it: a value, never a function. */ export interface HostLimitConfig { From 446817a75a7b33c4a5578782e9a7eb119e224b72 Mon Sep 17 00:00:00 2001 From: Hannah Wolfe Date: Mon, 28 Sep 2026 17:31:24 +0100 Subject: [PATCH 02/21] Moved magic-link helpers into a dedicated server library (#31028) Keep the services catalogue focused on owned application capabilities by moving shared magic-link support into `server/lib/magic-link`. Magic-link token creation, validation and URL generation are shared by members, newsletters, settings and welcome emails. They deserve a dedicated library rather than a home under services or mail delivery. This moves the implementation and unit tests and updates imports; the implementation is unchanged. --- .../core/server/{services => }/lib/magic-link/magic-link.js | 0 .../core/core/server/services/member-welcome-emails/service.js | 2 +- .../core/server/services/members/members-api/members-api.js | 2 +- .../core/server/services/newsletters/newsletters-service.js | 2 +- .../core/server/services/settings/settings-bread-service.js | 2 +- .../unit/server/{services => }/lib/magic-link/index.test.js | 2 +- .../server/services/members/members-api/members-api.test.js | 2 +- 7 files changed, 6 insertions(+), 6 deletions(-) rename ghost/core/core/server/{services => }/lib/magic-link/magic-link.js (100%) rename ghost/core/test/unit/server/{services => }/lib/magic-link/index.test.js (99%) diff --git a/ghost/core/core/server/services/lib/magic-link/magic-link.js b/ghost/core/core/server/lib/magic-link/magic-link.js similarity index 100% rename from ghost/core/core/server/services/lib/magic-link/magic-link.js rename to ghost/core/core/server/lib/magic-link/magic-link.js diff --git a/ghost/core/core/server/services/member-welcome-emails/service.js b/ghost/core/core/server/services/member-welcome-emails/service.js index 95bb4241cc7..e943a3a1cdd 100644 --- a/ghost/core/core/server/services/member-welcome-emails/service.js +++ b/ghost/core/core/server/services/member-welcome-emails/service.js @@ -3,7 +3,7 @@ const errors = require('@tryghost/errors'); const urlUtils = require('../../../shared/url-utils').default; const settingsCache = require('../../../shared/settings-cache'); const verifyEmailTemplate = require('../newsletters/emails/verify-email'); -const MagicLink = require('../lib/magic-link/magic-link'); +const MagicLink = require('../../lib/magic-link/magic-link'); const sentry = require('../../../shared/sentry'); const emailAddressService = require('../email-address'); const settingsHelpers = require('../settings-helpers'); diff --git a/ghost/core/core/server/services/members/members-api/members-api.js b/ghost/core/core/server/services/members/members-api/members-api.js index a94b44258c3..903a0fdf4b4 100644 --- a/ghost/core/core/server/services/members/members-api/members-api.js +++ b/ghost/core/core/server/services/members/members-api/members-api.js @@ -18,7 +18,7 @@ const MemberController = require('./controllers/member-controller'); const WellKnownController = require('./controllers/well-known-controller'); const { EmailSuppressedEvent } = require('../../email-suppression-list/email-suppression-list'); -const MagicLink = require('../../lib/magic-link/magic-link'); +const MagicLink = require('../../../lib/magic-link/magic-link'); const DomainEvents = require('@tryghost/domain-events'); const automationsApi = require('../../automations/automations-api'); diff --git a/ghost/core/core/server/services/newsletters/newsletters-service.js b/ghost/core/core/server/services/newsletters/newsletters-service.js index 7e9a78e2973..60b9401a1bd 100644 --- a/ghost/core/core/server/services/newsletters/newsletters-service.js +++ b/ghost/core/core/server/services/newsletters/newsletters-service.js @@ -6,7 +6,7 @@ const tpl = require('@tryghost/tpl'); const errors = require('@tryghost/errors'); const sentry = require('../../../shared/sentry'); -const MagicLink = require('../lib/magic-link/magic-link'); +const MagicLink = require('../../lib/magic-link/magic-link'); const messages = { nameAlreadyExists: 'A newsletter with the same name already exists', diff --git a/ghost/core/core/server/services/settings/settings-bread-service.js b/ghost/core/core/server/services/settings/settings-bread-service.js index fff5685a8b3..0aae9525dff 100644 --- a/ghost/core/core/server/services/settings/settings-bread-service.js +++ b/ghost/core/core/server/services/settings/settings-bread-service.js @@ -10,7 +10,7 @@ const { const { obfuscatedSetting, isSecretSetting, hideValueIfSecret } = require('./settings-utils'); const logging = require('@tryghost/logging'); const verifyEmailTemplate = require('./emails/verify-email'); -const MagicLink = require('../lib/magic-link/magic-link'); +const MagicLink = require('../../lib/magic-link/magic-link'); const sentry = require('../../../shared/sentry'); const EMAIL_KEYS = ['members_support_address']; diff --git a/ghost/core/test/unit/server/services/lib/magic-link/index.test.js b/ghost/core/test/unit/server/lib/magic-link/index.test.js similarity index 99% rename from ghost/core/test/unit/server/services/lib/magic-link/index.test.js rename to ghost/core/test/unit/server/lib/magic-link/index.test.js index e190ed23795..32b7535e722 100644 --- a/ghost/core/test/unit/server/services/lib/magic-link/index.test.js +++ b/ghost/core/test/unit/server/lib/magic-link/index.test.js @@ -1,6 +1,6 @@ const assert = require('node:assert/strict'); const sinon = require('sinon'); -const MagicLink = require('../../../../../../core/server/services/lib/magic-link/magic-link'); +const MagicLink = require('../../../../../core/server/lib/magic-link/magic-link'); const sandbox = sinon.createSandbox(); diff --git a/ghost/core/test/unit/server/services/members/members-api/members-api.test.js b/ghost/core/test/unit/server/services/members/members-api/members-api.test.js index aa606997040..d177fa20101 100644 --- a/ghost/core/test/unit/server/services/members/members-api/members-api.test.js +++ b/ghost/core/test/unit/server/services/members/members-api/members-api.test.js @@ -1,7 +1,7 @@ const assert = require('node:assert/strict'); const sinon = require('sinon'); const MembersAPI = require('../../../../../../core/server/services/members/members-api/members-api'); -const MagicLink = require('../../../../../../core/server/services/lib/magic-link/magic-link'); +const MagicLink = require('../../../../../../core/server/lib/magic-link/magic-link'); const GeolocationService = require('../../../../../../core/server/services/members/members-api/services/geolocation-service'); const MemberRepository = require('../../../../../../core/server/services/members/members-api/repositories/member-repository'); const MemberBREADService = require('../../../../../../core/server/services/members/members-api/services/member-bread-service'); From e3972e6b65d5b2d5c84d565805066730b1700daa Mon Sep 17 00:00:00 2001 From: Steve Larson <9larsons@gmail.com> Date: Mon, 28 Sep 2026 18:36:58 +0200 Subject: [PATCH 03/21] Added Sentry reporting for failed saves in the React editor (#30999) no ref This wires up Sentry to the same rough shape of logging and attribution that we used in the Ember implementation. --- apps/admin/src/editor/engine/README.md | 4 +- .../engine/__test-utils__/engine-harness.ts | 6 +- .../engine/save-engine.reporting.test.ts | 215 ++++++++++++ apps/admin/src/editor/engine/save-engine.ts | 70 +++- .../src/editor/feature-image-caption.tsx | 4 +- apps/admin/src/editor/koenig-post-editor.tsx | 4 +- apps/admin/src/editor/report-error.test.ts | 331 +++++++++++++++++- apps/admin/src/editor/report-error.ts | 117 ++++++- apps/admin/src/editor/session/README.md | 23 ++ .../session/editor-session.reporting.test.ts | 175 +++++++++ .../src/editor/session/editor-session.ts | 57 ++- .../editor/session/session-banners.test.tsx | 106 +++++- .../src/editor/session/session-banners.tsx | 35 +- .../session/use-editor-session.test.tsx | 42 +++ .../src/editor/session/use-editor-session.ts | 8 +- .../src/editor/settings/revision-preview.tsx | 4 +- .../components/error-boundary.test.tsx | 54 +++ .../settings/components/error-boundary.tsx | 14 +- 18 files changed, 1225 insertions(+), 44 deletions(-) create mode 100644 apps/admin/src/editor/engine/save-engine.reporting.test.ts create mode 100644 apps/admin/src/editor/session/editor-session.reporting.test.ts create mode 100644 apps/admin/src/settings/components/error-boundary.test.tsx diff --git a/apps/admin/src/editor/engine/README.md b/apps/admin/src/editor/engine/README.md index 008acba732b..a899bff8e8d 100644 --- a/apps/admin/src/editor/engine/README.md +++ b/apps/admin/src/editor/engine/README.md @@ -128,13 +128,15 @@ successful save or accepted reload releases the collision. Other states: `subscribe()` suits `useSyncExternalStore`: emissions are deduplicated, listeners receive the emitted value, a throwing `onStateChange` port or subscriber is reported through `onListenerError` without interrupting the save, and a nested transition ends the outer pass so no listener sees an out-of-order state. +`onSaveFailed` is called once per request that ran and failed, after the failure state is published (queued work the failure dropped is not reported): with the command it ran, the error, whether the post carried a server id, and how long `execute` took (null when the request never reached it). A local validation hold on background work is not a failure; a frozen request is reported only once re-authentication is abandoned. A throwing port is reported through `onListenerError`. + ## Change tracker `change-tracker.ts` + `lexical-compare.ts`. Answers one question for the editor: does the live post differ from what is persisted, and why. Pure, React-free; the editor's hidden second Koenig instance supplies the baseline. ### State model -Three documents: **saved** (last persisted state, from load/refetch/acknowledged save), **baseline** (the hidden instance's post-load serialization — the document after Lexical's load-time transforms), **live** (the visible editor). Body verdict: dirty ⇔ live differs from saved **and** from baseline. Baseline readiness is separate from its value: `pending` (not reported yet), `ready` (a known document; `null`, `''`, and an empty root are all known-empty), `failed`. A live edit while pending is dirty (`BASELINE_PENDING`, fail closed); a failed baseline falls back to live-vs-saved (`BASELINE_FAILED`) and never disables body protection. Title, the ordered tag list, and the editable attributes contribute their own dirty bits. Stable reason codes identify each cause (nothing reports them): `POST_HAS_ERROR`, `POST_TAGS_DIVERGED`, `POST_TITLE_DIVERGED`, `SCRATCH_DIVERGED_FROM_SECONDARY`, `NEW_POST_HAS_CHANGED_ATTRIBUTES`, `POST_HAS_DIRTY_ATTRIBUTES`, `BASELINE_PENDING`, `BASELINE_FAILED`, `LEXICAL_PARSE_FAILED` (malformed or structurally invalid Lexical is dirty, never a thrown route blocker). +Three documents: **saved** (last persisted state, from load/refetch/acknowledged save), **baseline** (the hidden instance's post-load serialization — the document after Lexical's load-time transforms), **live** (the visible editor). Body verdict: dirty ⇔ live differs from saved **and** from baseline. Baseline readiness is separate from its value: `pending` (not reported yet), `ready` (a known document; `null`, `''`, and an empty root are all known-empty), `failed`. A live edit while pending is dirty (`BASELINE_PENDING`, fail closed); a failed baseline falls back to live-vs-saved (`BASELINE_FAILED`) and never disables body protection. Title, the ordered tag list, and the editable attributes contribute their own dirty bits. Stable reason codes identify each cause, which the session reports when a leave has to be confirmed: `POST_HAS_ERROR`, `POST_TAGS_DIVERGED`, `POST_TITLE_DIVERGED`, `SCRATCH_DIVERGED_FROM_SECONDARY`, `NEW_POST_HAS_CHANGED_ATTRIBUTES`, `POST_HAS_DIRTY_ATTRIBUTES`, `BASELINE_PENDING`, `BASELINE_FAILED`, `LEXICAL_PARSE_FAILED` (malformed or structurally invalid Lexical is dirty, never a thrown route blocker). ### API (id-first; events for another post are dropped) diff --git a/apps/admin/src/editor/engine/__test-utils__/engine-harness.ts b/apps/admin/src/editor/engine/__test-utils__/engine-harness.ts index 98c6cb53eb9..8134584be1f 100644 --- a/apps/admin/src/editor/engine/__test-utils__/engine-harness.ts +++ b/apps/admin/src/editor/engine/__test-utils__/engine-harness.ts @@ -7,6 +7,7 @@ import { type SaveEngine, type SaveEngineState, type SaveError, + type SaveFailure, type SaveOutcome, type SaveRequest, type SaveResult, @@ -57,7 +58,10 @@ export function dispatchAny(engine: SaveEngine, kind: DispatchIntent) { export function setup( overrides: Partial = {}, - ports: { autosaveDebounceMs?: () => number | undefined } = {}, + ports: { + autosaveDebounceMs?: () => number | undefined; + onSaveFailed?: (failure: SaveFailure) => void; + } = {}, ) { let snapshot = { ...BASE, ...overrides } as SaveSnapshot; const requests: SaveRequest[] = []; diff --git a/apps/admin/src/editor/engine/save-engine.reporting.test.ts b/apps/admin/src/editor/engine/save-engine.reporting.test.ts new file mode 100644 index 00000000000..4a1688beec2 --- /dev/null +++ b/apps/admin/src/editor/engine/save-engine.reporting.test.ts @@ -0,0 +1,215 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { createSaveEngine, type SaveFailure, type SaveRequest } from './save-engine'; +import { + conflict, + flush, + hostLimit, + notFound, + setup, + sessionInvalid, + transport, + unknown, + validation, +} from './__test-utils__/engine-harness'; + +beforeEach(() => { + vi.useFakeTimers(); +}); + +afterEach(() => { + vi.useRealTimers(); +}); + +function reporting(overrides: Parameters[0] = {}) { + const failures: SaveFailure[] = []; + const h = setup(overrides, { onSaveFailed: (failure) => failures.push(failure) }); + return { ...h, failures }; +} + +describe('createSaveEngine onSaveFailed', () => { + it.each([transport, unknown, hostLimit, validation])( + 'reports a $kind failure from execute once, with the request and its timing', + async (error) => { + const h = reporting(); + void h.engine.dispatch('explicit'); + await flush(); + vi.advanceTimersByTime(2500); + + await h.fail(error); + + expect(h.failures).toEqual([ + { + command: { kind: 'explicit', requiresRevision: true, requiresReconfirmation: false }, + error, + persisted: true, + durationMs: 2500, + }, + ]); + }, + ); + + it('reports nothing for a save that succeeds', async () => { + const h = reporting(); + void h.engine.dispatch('explicit'); + + await h.succeed(); + + expect(h.failures).toEqual([]); + }); + + it('reports a rejected execute as the unknown failure it became', async () => { + const h = reporting(); + const cause = new Error('network down'); + void h.engine.dispatch('field'); + + await h.reject(cause); + + expect(h.failures).toHaveLength(1); + expect(h.failures[0]).toMatchObject({ + command: { kind: 'field' }, + error: { kind: 'unknown', message: 'network down', cause }, + }); + }); + + it('reports a not-found with whether the post had an id', async () => { + const h = reporting({ id: null, updatedAt: null }); + void h.engine.dispatch('autosave'); + await vi.advanceTimersByTimeAsync(3000); + + await h.fail(notFound); + + expect(h.failures).toHaveLength(1); + expect(h.failures[0]).toMatchObject({ error: notFound, persisted: false }); + }); + + it('reports a collision and not the queued saves it dropped', async () => { + const h = reporting(); + void h.engine.dispatch('field'); + await flush(); + void h.engine.dispatch('explicit'); + + await h.fail(conflict); + + expect(h.failures).toHaveLength(1); + expect(h.failures[0]).toMatchObject({ command: { kind: 'field' }, error: conflict }); + }); + + it('reports an expired session only once re-authentication is abandoned', async () => { + const h = reporting(); + void h.engine.dispatch('explicit'); + await flush(); + vi.advanceTimersByTime(400); + await h.fail(sessionInvalid); + expect(h.engine.getState()).toEqual({ kind: 'reauth-pending', intent: 'explicit' }); + expect(h.failures).toEqual([]); + + // The report describes the request as it ran, not the post as it is now. + h.patch({ id: null, updatedAt: null }); + h.engine.reauthAbandoned(); + + expect(h.failures).toHaveLength(1); + expect(h.failures[0]).toMatchObject({ + error: sessionInvalid, + persisted: true, + durationMs: 400, + }); + }); + + it('reports nothing when re-authentication succeeds and the save is retried', async () => { + const h = reporting(); + void h.engine.dispatch('explicit'); + await h.fail(sessionInvalid); + + h.engine.reauthSucceeded(); + await h.succeed(); + + expect(h.failures).toEqual([]); + }); + + it('reports a prepare failure without a duration, since no request was sent', async () => { + const h = reporting(); + h.prepare.mockResolvedValueOnce({ ok: false, error: unknown }); + + await h.engine.dispatch('explicit'); + + expect(h.execute).not.toHaveBeenCalled(); + expect(h.failures).toHaveLength(1); + expect(h.failures[0]).toMatchObject({ error: unknown, durationMs: null }); + }); + + it('reports a local validation failure for an explicit save but not a held background save', async () => { + const h = reporting(); + h.prepare.mockResolvedValue({ ok: false, error: validation }); + + await expect(h.engine.dispatch('field')).resolves.toEqual({ + kind: 'blocked', + error: validation, + }); + expect(h.failures).toEqual([]); + + await h.engine.dispatch('explicit'); + + expect(h.failures).toHaveLength(1); + expect(h.failures[0]).toMatchObject({ error: validation, durationMs: null }); + }); + + it('reports a snapshot that could not be read', async () => { + const h = reporting(); + const cause = new Error('snapshot exploded'); + h.throwNextSnapshot(cause); + + await h.engine.dispatch('explicit'); + + expect(h.failures).toHaveLength(1); + expect(h.failures[0]).toMatchObject({ + error: { kind: 'unknown', message: 'snapshot exploded', cause }, + persisted: true, + }); + }); + + it('routes a throwing reporter to onListenerError and still settles the save', async () => { + const h = setup( + {}, + { + onSaveFailed: () => { + throw new Error('reporter down'); + }, + }, + ); + const completion = h.engine.dispatch('explicit'); + + await h.fail(transport); + + await expect(completion).resolves.toEqual({ + kind: 'failed', + error: transport, + executedAs: 'explicit', + }); + expect(h.listenerErrors).toEqual([new Error('reporter down')]); + }); + + it('is optional', async () => { + const engine = createSaveEngine({ + getSnapshot: () => ({ + id: 'post-1', + updatedAt: '2026-09-02T11:00:00.000Z', + status: 'draft', + publishedAt: null, + title: 'Hello', + slug: 'hello', + isDirty: true, + changedSinceLastRevision: true, + version: 1, + }), + slug: { + settled: () => Promise.resolve(), + fromTitle: () => Promise.resolve({ slug: '', source: 'unchanged' }), + }, + prepare: (request: SaveRequest) => Promise.resolve({ ok: true, prepared: request }), + execute: () => Promise.resolve({ ok: false, error: transport }), + reconcile: () => {}, + }); + + await expect(engine.dispatch('explicit')).resolves.toMatchObject({ kind: 'failed' }); + }); +}); diff --git a/apps/admin/src/editor/engine/save-engine.ts b/apps/admin/src/editor/engine/save-engine.ts index 9f2cd669fd1..5a16c3ae49f 100644 --- a/apps/admin/src/editor/engine/save-engine.ts +++ b/apps/admin/src/editor/engine/save-engine.ts @@ -133,6 +133,16 @@ export type SaveOutcome = /** Local validation holds background work; other failures use the normal error handling. */ export type PrepareOutcome

= { ok: true; prepared: P } | { ok: false; error: SaveError }; +/** A request that settled as `failed`, once per request. */ +export interface SaveFailure { + readonly command: SaveCommand; + readonly error: SaveError; + /** Whether the post carried a server id when the request ran. */ + readonly persisted: boolean; + /** Milliseconds `execute` took before failing; null when the request never reached it. */ + readonly durationMs: number | null; +} + /** Unsaved content is independent of the commands currently allowed to execute. */ export interface PendingSave { blockedBy: SaveError | null; @@ -202,6 +212,8 @@ export interface SaveEnginePorts< onStateChange?: (state: SaveEngineState) => void; /** A throwing state callback or subscriber is reported here instead of interrupting the save. */ onListenerError?: (error: unknown) => void; + /** Called after a failed request has settled; a throw is reported like a listener's. */ + onSaveFailed?: (failure: SaveFailure) => void; } export interface SaveEngine { @@ -311,6 +323,8 @@ interface Timer { interface Frozen { slot: Slot; error: SaveError; + persisted: boolean; + durationMs: number | null; } const AUTOSAVE: SaveCommand = { @@ -582,12 +596,43 @@ export function createSaveEngine< : { kind: 'error', intent, error }; } - function failSlot(slot: Slot, error: SaveError): void { + function failSlot( + slot: Slot, + error: SaveError, + snapshot: S | null, + durationMs: number | null, + ): void { settle(slot.waiters, failed(error, slot.command.kind)); setState(failureState(slot.command.kind, error)); + reportFailure(slot, error, snapshot, durationMs); drain(); } + function reportFailure( + slot: Slot, + error: SaveError, + snapshot: S | null, + durationMs: number | null, + ): void { + report({ + command: slot.command, + error, + persisted: snapshot !== null && snapshot.id !== null, + durationMs, + }); + } + + function report(failure: SaveFailure): void { + if (!ports.onSaveFailed) { + return; + } + try { + ports.onSaveFailed(failure); + } catch (cause) { + reportListenerError(cause); + } + } + function blankToDefault(title: string): string { return title.trim() ? title : DEFAULT_TITLE; } @@ -655,7 +700,7 @@ export function createSaveEngine< try { snapshot = ports.getSnapshot(); } catch (cause) { - failSlot(slot, toSaveError(cause)); + failSlot(slot, toSaveError(cause), readSnapshot(), null); return; } const early = dropReason(slot, snapshot); @@ -671,6 +716,7 @@ export function createSaveEngine< setState(deriveState()); let outcome: SaveOutcome; + let executeStartedAt: number | null = null; try { await ports.slug.settled(); if (disposed) { @@ -717,6 +763,7 @@ export function createSaveEngine< ); if (!isBackgroundIntent(slot.command.kind)) { setState(failureState(slot.command.kind, preparation.error)); + reportFailure(slot, preparation.error, snapshot, null); } drain(); return; @@ -731,6 +778,7 @@ export function createSaveEngine< if (disposed) { return; } + executeStartedAt = Date.now(); outcome = await ports.execute(preparation.prepared, abort.signal); if (disposed) { return; @@ -760,14 +808,19 @@ export function createSaveEngine< drain(); return; } - handleError(slot, snapshot, outcome.error); + handleError( + slot, + snapshot, + outcome.error, + executeStartedAt === null ? null : Date.now() - executeStartedAt, + ); } - function handleError(slot: Slot, snapshot: S, error: SaveError): void { + function handleError(slot: Slot, snapshot: S, error: SaveError, durationMs: number | null): void { const intent = slot.command.kind; if (error.kind === 'session-invalid') { - frozen = { slot, error }; + frozen = { slot, error, persisted: snapshot.id !== null, durationMs }; setState({ kind: 'reauth-pending', intent }); return; } @@ -782,6 +835,7 @@ export function createSaveEngine< settle(dropWaiters, dropped('halted')); settle(slot.waiters, failed(error, intent)); setState({ kind: snapshot.id ? 'halted' : 'crashed' }); + reportFailure(slot, error, snapshot, durationMs); return; } @@ -797,6 +851,7 @@ export function createSaveEngine< settle(dropWaiters, dropped('conflict')); settle(slot.waiters, failed(error, intent)); setState({ kind: 'conflict', intent, error }); + reportFailure(slot, error, snapshot, durationMs); return; } @@ -807,7 +862,7 @@ export function createSaveEngine< ) { hold = { version: snapshot.version, source: 'server', error }; } - failSlot(slot, error); + failSlot(slot, error, snapshot, durationMs); } function captureCommand(kind: DispatchIntent, snapshot: S | null, options?: PublishOptions) { @@ -947,7 +1002,7 @@ export function createSaveEngine< if (!frozen || disposed) { return; } - const { slot, error } = frozen; + const { slot, error, persisted, durationMs } = frozen; frozen = null; settle(slot.waiters, failed(error, slot.command.kind)); const waiters: Waiter[] = []; @@ -960,6 +1015,7 @@ export function createSaveEngine< waiter.resolve(failed(error, waiter.command.kind)); } setState(failureState(slot.command.kind, error)); + report({ command: slot.command, error, persisted, durationMs }); } // A server document that no longer carries the rejected updated_at ends the diff --git a/apps/admin/src/editor/feature-image-caption.tsx b/apps/admin/src/editor/feature-image-caption.tsx index b1e773a1af8..d2d92096580 100644 --- a/apps/admin/src/editor/feature-image-caption.tsx +++ b/apps/admin/src/editor/feature-image-caption.tsx @@ -6,7 +6,7 @@ import { loadKoenig, } from '@/settings/components/koenig-loader'; import type { PostCardConfig } from './card-config'; -import { reportKoenigError } from './report-error'; +import { reportKoenigError, reportKoenigRenderError } from './report-error'; export interface FeatureImageCaptionProps { /** Paragraph-wrapped caption HTML; the editor parses it as a document. */ @@ -76,7 +76,7 @@ export function FeatureImageCaption(props: FeatureImageCaptionProps) { return (

- + diff --git a/apps/admin/src/editor/koenig-post-editor.tsx b/apps/admin/src/editor/koenig-post-editor.tsx index da44ac80c62..5c243f462f1 100644 --- a/apps/admin/src/editor/koenig-post-editor.tsx +++ b/apps/admin/src/editor/koenig-post-editor.tsx @@ -9,7 +9,7 @@ import { } from '@/settings/components/koenig-loader'; import type { PostCardConfig } from './card-config'; import { editorFileUploader } from './koenig-file-uploader'; -import { reportKoenigError } from './report-error'; +import { reportKoenigError, reportKoenigRenderError } from './report-error'; const NOOP = () => {}; @@ -99,7 +99,7 @@ export const KoenigPostEditor = memo(function KoenigPostEditor(props: KoenigPost return (
- + diff --git a/apps/admin/src/editor/report-error.test.ts b/apps/admin/src/editor/report-error.test.ts index 9dccdacd995..710964723b1 100644 --- a/apps/admin/src/editor/report-error.test.ts +++ b/apps/admin/src/editor/report-error.test.ts @@ -1,19 +1,57 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import * as Sentry from '@sentry/react'; -import { reportEditorError, reportKoenigError } from './report-error'; +import { APIError, ServerUnreachableError } from '@tryghost/admin-x-framework/errors'; +import type { SaveCommand, SaveError } from '@/editor/engine/save-engine'; +import type { EditorSaveFailure } from '@/editor/session/editor-session'; +import { + reportEditorError, + reportKoenigError, + reportKoenigRenderError, + reportLeaveConfirmation, + reportSaveFailure, + reportShownAlert, +} from './report-error'; -vi.mock('@sentry/react', () => ({ captureException: vi.fn() })); +vi.mock('@sentry/react', () => ({ captureException: vi.fn(), captureMessage: vi.fn() })); -describe('reportEditorError', () => { - beforeEach(() => { - vi.spyOn(console, 'error').mockImplementation(() => {}); - }); +const FIELD: SaveCommand = { + kind: 'field', + requiresRevision: false, + requiresReconfirmation: false, +}; - afterEach(() => { - vi.restoreAllMocks(); - vi.mocked(Sentry.captureException).mockClear(); - }); +function failure(overrides: Partial = {}): EditorSaveFailure { + return { + command: FIELD, + error: { kind: 'unknown', message: 'Boom', cause: new Error('Boom') }, + persisted: true, + durationMs: 120, + postId: 'post-1', + status: 'draft', + ...overrides, + }; +} + +const TAGS = { + savePostTask: true, + post_type: 'post', + save_intent: 'field', + save_error_kind: 'unknown', + save_persisted: true, + save_status: 'draft', +}; + +beforeEach(() => { + vi.spyOn(console, 'error').mockImplementation(() => {}); +}); +afterEach(() => { + vi.restoreAllMocks(); + vi.mocked(Sentry.captureException).mockClear(); + vi.mocked(Sentry.captureMessage).mockClear(); +}); + +describe('reportEditorError', () => { it('forwards the error to Sentry with the given context and logs it once', () => { const error = new Error('boom'); @@ -47,3 +85,276 @@ describe('reportEditorError', () => { }); }); }); + +describe('reportKoenigRenderError', () => { + it('tags a boundary crash as Lexical and keeps where in the tree it happened', () => { + window['@tryghost/koenig-lexical'] = { version: '1.2.3' }; + const error = new Error('render exploded'); + + reportKoenigRenderError(error, { componentStack: '\n at KoenigComposer' }); + + expect(Sentry.captureException).toHaveBeenCalledWith(error, { + tags: { lexical: true }, + contexts: { + koenig: { version: '1.2.3' }, + react: { componentStack: '\n at KoenigComposer' }, + }, + }); + }); +}); + +describe('reportSaveFailure', () => { + it('reports the failing request with what it was and which post it was for', () => { + const cause = new Error('Boom'); + + reportSaveFailure(failure({ error: { kind: 'unknown', message: 'Boom', cause } }), 'post'); + + expect(Sentry.captureException).toHaveBeenCalledTimes(1); + expect(Sentry.captureException).toHaveBeenCalledWith(cause, { + tags: TAGS, + extra: { post_id: 'post-1', duration_ms: 120 }, + }); + // eslint-disable-next-line no-console + expect(console.error).toHaveBeenCalledWith(cause); + }); + + it('builds an error from the message when the failure has no cause', () => { + reportSaveFailure( + failure({ error: { kind: 'unknown', message: 'No record came back' } }), + 'page', + ); + + expect(Sentry.captureException).toHaveBeenCalledWith(new Error('No record came back'), { + tags: { ...TAGS, post_type: 'page' }, + extra: { post_id: 'post-1', duration_ms: 120 }, + }); + }); + + it.each<[string, SaveCommand['kind']]>([ + ['an autosave', 'autosave'], + ['the timed cycle', 'timed'], + ['a leave save', 'leave'], + ['a publish', 'publish'], + ])('tags a failure from %s with its intent', (_label, kind) => { + reportSaveFailure(failure({ command: { ...FIELD, kind } }), 'post'); + + expect(Sentry.captureException).toHaveBeenCalledTimes(1); + expect(vi.mocked(Sentry.captureException).mock.calls[0][1]).toMatchObject({ + tags: { save_intent: kind }, + }); + }); + + it('reports a collision with its kind and the persisted status', () => { + const cause = new Error('Saving failed! Someone else is editing this post.'); + + reportSaveFailure( + failure({ + command: { ...FIELD, kind: 'explicit', requiresRevision: true }, + error: { kind: 'conflict', message: cause.message, cause }, + status: 'published', + }), + 'post', + ); + + expect(Sentry.captureException).toHaveBeenCalledWith(cause, { + tags: { + ...TAGS, + save_intent: 'explicit', + save_error_kind: 'conflict', + save_status: 'published', + }, + extra: { post_id: 'post-1', duration_ms: 120 }, + }); + }); + + it('reports an abandoned re-authentication as the session failure it was', () => { + const cause = new Error('Unauthorized'); + + reportSaveFailure( + failure({ error: { kind: 'session-invalid', message: 'Unauthorized', cause } }), + 'post', + ); + + expect(Sentry.captureException).toHaveBeenCalledWith(cause, { + tags: { ...TAGS, save_error_kind: 'session-invalid' }, + extra: { post_id: 'post-1', duration_ms: 120 }, + }); + }); + + it('reports a persisted post that is gone as a message carrying the post id', () => { + reportSaveFailure( + failure({ error: { kind: 'not-found', message: 'Post not found', cause: new Error('404') } }), + 'page', + ); + + expect(Sentry.captureException).not.toHaveBeenCalled(); + expect(Sentry.captureMessage).toHaveBeenCalledTimes(1); + expect(Sentry.captureMessage).toHaveBeenCalledWith('Attempted to edit deleted page', { + tags: { ...TAGS, post_type: 'page', save_error_kind: 'not-found' }, + extra: { post_id: 'post-1' }, + }); + }); + + it('reports a not-found on an unpersisted post as an exception', () => { + const cause = new Error('404'); + + reportSaveFailure( + failure({ + error: { kind: 'not-found', message: 'Post not found', cause }, + persisted: false, + postId: null, + }), + 'post', + ); + + expect(Sentry.captureMessage).not.toHaveBeenCalled(); + expect(Sentry.captureException).toHaveBeenCalledWith(cause, { + tags: { ...TAGS, save_error_kind: 'not-found', save_persisted: false }, + extra: { post_id: null, duration_ms: 120 }, + }); + }); + + it.each<[string, SaveError]>([ + ['a validation failure', { kind: 'validation', message: 'Title is too long' }], + ['a host limit', { kind: 'host-limit', message: 'Upgrade required' }], + [ + 'an unreachable server', + { kind: 'transport', message: 'Unreachable', cause: new ServerUnreachableError() }, + ], + ])('sends nothing for %s', (_label, error) => { + reportSaveFailure(failure({ error }), 'post'); + + expect(Sentry.captureException).not.toHaveBeenCalled(); + expect(Sentry.captureMessage).not.toHaveBeenCalled(); + // eslint-disable-next-line no-console + expect(console.error).not.toHaveBeenCalled(); + }); + + it('reports a failure that took more than two seconds with its timing as well', () => { + const cause = new ServerUnreachableError(); + + reportSaveFailure( + failure({ + command: { + kind: 'publish', + requiresRevision: true, + requiresReconfirmation: true, + target: { status: 'published', publishedAt: null, emailSegment: 'status:free' }, + }, + error: { kind: 'transport', message: 'Unreachable', cause }, + durationMs: 2001, + }), + 'post', + ); + + expect(Sentry.captureException).toHaveBeenCalledTimes(1); + expect(Sentry.captureException).toHaveBeenCalledWith('Failed Lexical save took > 2s', { + tags: { + ...TAGS, + save_intent: 'publish', + save_error_kind: 'transport', + save_time: 3, + save_revision: true, + email_segment: 'status:free', + }, + extra: { post_id: 'post-1' }, + }); + }); + + it('omits the email segment tag from a slow failure that carried none', () => { + reportSaveFailure(failure({ durationMs: 2001 }), 'post'); + + expect(Sentry.captureException).toHaveBeenCalledTimes(2); + expect(vi.mocked(Sentry.captureException).mock.calls[0][1]).toStrictEqual({ + tags: { ...TAGS, save_time: 3, save_revision: false }, + extra: { post_id: 'post-1' }, + }); + }); + + it('sends no timing for a failure that never reached the transport', () => { + reportSaveFailure(failure({ durationMs: null }), 'post'); + + expect(Sentry.captureException).toHaveBeenCalledTimes(1); + expect(vi.mocked(Sentry.captureException).mock.calls[0][1]).toMatchObject({ + extra: { post_id: 'post-1', duration_ms: null }, + }); + }); +}); + +describe('reportSaveFailure response tags', () => { + it('tags a failure the server answered with its status', () => { + const cause = new APIError(new Response(null, { status: 500 })); + + reportSaveFailure(failure({ error: { kind: 'unknown', message: 'Boom', cause } }), 'post'); + + expect(Sentry.captureException).toHaveBeenCalledWith(cause, { + tags: { ...TAGS, api_response_status: 500 }, + extra: { post_id: 'post-1', duration_ms: 120 }, + }); + }); + + it('carries no response tags for a failure that never got an answer', () => { + reportSaveFailure( + failure({ error: { kind: 'unknown', message: 'Boom', cause: new Error() } }), + 'post', + ); + + const context = vi.mocked(Sentry.captureException).mock.calls[0][1] as { tags: object }; + expect(Object.keys(context.tags)).not.toContain('api_response_status'); + expect(Object.keys(context.tags)).not.toContain('api_url'); + }); +}); + +describe('reportShownAlert', () => { + it('reports the banner text the writer read with the failure behind it', () => { + const cause = new APIError(new Response(null, { status: 409 })); + + reportShownAlert('Someone else is editing this post.', { + kind: 'conflict', + message: 'Saving failed!', + cause, + }); + + expect(Sentry.captureMessage).toHaveBeenCalledTimes(1); + expect(Sentry.captureMessage).toHaveBeenCalledWith('Someone else is editing this post.', { + tags: { + shown_to_user: true, + source: 'editor-banner', + save_error_kind: 'conflict', + api_response_status: 409, + }, + contexts: { + ghost: { + displayed_message: 'Someone else is editing this post.', + save_error_message: 'Saving failed!', + }, + }, + }); + expect(Sentry.captureException).not.toHaveBeenCalled(); + }); +}); + +describe('reportLeaveConfirmation', () => { + it('reports the leave prompt with why the post counted as unsaved', () => { + reportLeaveConfirmation( + { + postId: 'post-1', + status: 'draft', + engineState: 'error', + reasons: ['POST_HAS_ERROR', 'SCRATCH_DIVERGED_FROM_SECONDARY'], + }, + 'post', + ); + + expect(Sentry.captureMessage).toHaveBeenCalledTimes(1); + expect(Sentry.captureMessage).toHaveBeenCalledWith('showing leave editor modal', { + tags: { + post_type: 'post', + save_status: 'draft', + engine_state: 'error', + leave_reasons: 'POST_HAS_ERROR,SCRATCH_DIVERGED_FROM_SECONDARY', + }, + extra: { post_id: 'post-1', reasons: ['POST_HAS_ERROR', 'SCRATCH_DIVERGED_FROM_SECONDARY'] }, + }); + }); +}); diff --git a/apps/admin/src/editor/report-error.ts b/apps/admin/src/editor/report-error.ts index 954d119ddb1..bbebb4afb7b 100644 --- a/apps/admin/src/editor/report-error.ts +++ b/apps/admin/src/editor/report-error.ts @@ -1,10 +1,21 @@ import * as Sentry from '@sentry/react'; +import type { ErrorInfo } from 'react'; +import { APIError, ServerUnreachableError } from '@tryghost/admin-x-framework/errors'; +import type { PostType } from '@/editor/card-config'; +import type { SaveError } from '@/editor/engine/save-engine'; +import type { EditorLeaveConfirmation, EditorSaveFailure } from '@/editor/session/editor-session'; + +type TagValue = boolean | number | string; export interface EditorErrorContext { - tags?: Record; + tags?: Record; + extra?: Record; contexts?: Record>; } +/** A failed save is slow past this; Sentry gets a second event with its timing. */ +const SLOW_SAVE_MS = 2000; + /** * Reports an editor failure. Never rethrown: the editor recovers without losing * what the writer typed. @@ -23,3 +34,107 @@ export function reportKoenigError(error: unknown): void { contexts: { koenig: { version: window['@tryghost/koenig-lexical']?.version } }, }); } + +/** Reports a Koenig instance that crashed its error boundary, with where in the tree. */ +export function reportKoenigRenderError(error: unknown, info: ErrorInfo): void { + reportEditorError(error, { + tags: { lexical: true }, + contexts: { + koenig: { version: window['@tryghost/koenig-lexical']?.version }, + react: { componentStack: info.componentStack }, + }, + }); +} + +function definedTags(tags: Record): Record { + return Object.fromEntries( + Object.entries(tags).filter(([, value]) => value !== undefined), + ) as Record; +} + +/** The response behind a failure, when the transport answered at all. */ +function responseTags(error: SaveError): Record { + const response = error.cause instanceof APIError ? error.cause.response : undefined; + return { + api_response_status: response?.status, + // Sentry drops a tag value longer than 200 characters. + api_url: response?.url ? response.url.slice(0, 200) : undefined, + }; +} + +/** + * Reports a request that settled as failed. Validation, host limits and an + * unreachable server are the writer's or the host's to act on and are not + * reported; every other failure is, once, with what the request was. + */ +export function reportSaveFailure(failure: EditorSaveFailure, postType: PostType): void { + const { command, error, persisted, durationMs, postId, status } = failure; + const tags = definedTags({ + savePostTask: true, + post_type: postType, + save_intent: command.kind, + save_error_kind: error.kind, + save_persisted: persisted, + save_status: status, + ...responseTags(error), + }); + + if (durationMs !== null && durationMs > SLOW_SAVE_MS) { + Sentry.captureException('Failed Lexical save took > 2s', { + tags: definedTags({ + ...tags, + save_time: Math.ceil(durationMs / 1000), + save_revision: command.requiresRevision, + email_segment: command.target?.emailSegment, + }), + extra: { post_id: postId }, + }); + } + + if ( + error.kind === 'validation' || + error.kind === 'host-limit' || + error.cause instanceof ServerUnreachableError + ) { + return; + } + + if (error.kind === 'not-found' && persisted) { + Sentry.captureMessage(`Attempted to edit deleted ${postType}`, { + tags, + extra: { post_id: postId }, + }); + return; + } + + reportEditorError(error.cause ?? new Error(error.message), { + tags, + extra: { post_id: postId, duration_ms: durationMs }, + }); +} + +/** Reports an error banner the writer was shown, by the text they read. */ +export function reportShownAlert(message: string, error: SaveError): void { + Sentry.captureMessage(message, { + tags: definedTags({ + shown_to_user: true, + source: 'editor-banner', + save_error_kind: error.kind, + ...responseTags(error), + }), + contexts: { ghost: { displayed_message: message, save_error_message: error.message } }, + }); +} + +/** Reports a leave the writer had to confirm, with why the post counted as unsaved. */ +export function reportLeaveConfirmation(leave: EditorLeaveConfirmation, postType: PostType): void { + Sentry.captureMessage('showing leave editor modal', { + tags: { + post_type: postType, + save_status: leave.status, + engine_state: leave.engineState, + leave_reasons: leave.reasons.join(','), + }, + extra: { post_id: leave.postId, reasons: leave.reasons }, + }); +} diff --git a/apps/admin/src/editor/session/README.md b/apps/admin/src/editor/session/README.md index d30b250ddb7..bda73022ca4 100644 --- a/apps/admin/src/editor/session/README.md +++ b/apps/admin/src/editor/session/README.md @@ -238,6 +238,29 @@ references are kept stable across engine events, so body edits need no new React snapshot while the rendered values stay the same. That makes the view suitable for `useSyncExternalStore` and lets it stand in for those values as a dependency. +## What the session reports + +Failures never reach the writer as thrown errors; the session reports them. Every +request that ran and failed is reported once, with the command it ran, the +error, whether the post already had a server id, the post's persisted status, +the id, and how long the request took. Queued work a failure dropped is not +reported on its own. An expired session is reported only when re-authentication +is abandoned, not when it is retried. A leave the writer has to +confirm is reported with the reason codes the tracker holds the post dirty for. +A draft disposed with a title but a slug still derived from the default title is +reported as an error. A throwing subscriber or slug listener is reported as an +error, and so is a slug edit the generator rejected. + +Sentry receives these through the editor's own reporter, with the response +status and URL when the transport answered. Validation failures, host limits and +an unreachable server are not sent: they are the writer's or the host's to act +on. A failed request that took more than two seconds is sent as a second event +with its timing. Every error banner the writer is shown — a failed save, a +collision, a deleted post — is also sent once as a message carrying the text +they read. A Koenig instance that crashes its error boundary is reported as a +Lexical failure. Sentry stays optional: without a DSN the calls are no-ops, and +an error is still logged to the console. + ## The autosave debounce The autosave debounce is a boot value: the session reads it from the config the diff --git a/apps/admin/src/editor/session/editor-session.reporting.test.ts b/apps/admin/src/editor/session/editor-session.reporting.test.ts new file mode 100644 index 00000000000..5f500b5a4ea --- /dev/null +++ b/apps/admin/src/editor/session/editor-session.reporting.test.ts @@ -0,0 +1,175 @@ +import { describe, expect, it, vi } from 'vitest'; +import { SessionExpiredError } from '@tryghost/admin-x-framework/errors'; +import { DEFAULT_TITLE } from '@/editor/engine/save-engine'; +import { + body, + record, + sessionHarness, + updateCollision, +} from '@/editor/session/__test-utils__/session-harness'; +import type { EditorSaveFailure, EditorLeaveConfirmation } from './editor-session'; + +function reporting(...args: Parameters) { + const failures: EditorSaveFailure[] = []; + const leaves: EditorLeaveConfirmation[] = []; + const [options = {}, hooks] = args; + const harness = sessionHarness( + { + ...options, + onSaveFailed: (failure) => failures.push(failure), + onLeaveConfirmed: (leave) => leaves.push(leave), + }, + hooks, + ); + return { ...harness, failures, leaves }; +} + +describe('createEditorSession reporting', () => { + it('reports a failed update with the post id and its persisted status', async () => { + const collision = updateCollision(); + const { session, failures } = reporting( + { record: record({ status: 'published' }) }, + { failUpdateWith: collision }, + ); + session.patchTitle('Edited'); + + await session.dispatchExplicit(); + + expect(failures).toHaveLength(1); + expect(failures[0]).toMatchObject({ + command: { kind: 'explicit', requiresRevision: true, requiresReconfirmation: false }, + error: { kind: 'conflict', message: collision.message, cause: collision }, + persisted: true, + postId: 'abc123', + status: 'published', + }); + expect(typeof failures[0].durationMs).toBe('number'); + }); + + it('reports a create that answered with nothing as unpersisted', async () => { + const { session, failures, create } = reporting(); + create.mockResolvedValueOnce(undefined as never); + session.patchTitle('New'); + + await session.dispatchExplicit(); + + expect(failures).toHaveLength(1); + expect(failures[0]).toMatchObject({ + error: { kind: 'unknown', message: 'Couldn’t save this post.' }, + persisted: false, + postId: null, + status: 'draft', + }); + }); + + it('reports an expired session only once re-authentication is abandoned', async () => { + const { session, failures } = reporting( + { record: record() }, + { failUpdateWith: new SessionExpiredError(new Response(null, { status: 401 }), undefined) }, + ); + session.patchTitle('Edited'); + const completion = session.dispatchExplicit(); + await vi.waitFor(() => expect(session.getState().kind).toBe('reauth-pending')); + expect(failures).toEqual([]); + + session.reauthAbandoned(); + await completion; + + expect(failures).toHaveLength(1); + expect(failures[0]).toMatchObject({ error: { kind: 'session-invalid' } }); + }); + + it('reports a leave the writer has to confirm with why the post is dirty', async () => { + const { session, leaves } = reporting( + { record: record({ status: 'published' }) }, + { failUpdateWith: updateCollision() }, + ); + session.setBaseline(record().lexical); + session.patchLexical(body('Hello and more')); + await session.dispatchExplicit(); + + expect(await session.leaveRequested()).toBe('confirm'); + + expect(leaves).toEqual([ + { + postId: 'abc123', + status: 'published', + engineState: 'conflict', + reasons: ['POST_HAS_ERROR', 'SCRATCH_DIVERGED_FROM_SECONDARY'], + }, + ]); + }); + + it('reports nothing for a leave decided after the session was disposed', async () => { + const { session, leaves } = reporting( + { record: record({ status: 'published' }) }, + { failUpdateWith: updateCollision() }, + ); + session.patchTitle('Edited'); + await session.dispatchExplicit(); + + const pending = session.leaveRequested(); + session.dispose(); + + expect(await pending).toBe('confirm'); + expect(leaves).toEqual([]); + }); + + it('routes a throwing leave reporter to onError and still answers', async () => { + const onError = vi.fn(); + const { session } = sessionHarness( + { + record: record({ status: 'published' }), + onError, + onLeaveConfirmed: () => { + throw new Error('reporter down'); + }, + }, + { failUpdateWith: updateCollision() }, + ); + session.patchTitle('Edited'); + await session.dispatchExplicit(); + onError.mockClear(); + + expect(await session.leaveRequested()).toBe('confirm'); + + expect(onError).toHaveBeenCalledWith(new Error('reporter down')); + }); + + it('reports nothing for a leave that loses nothing', async () => { + const { session, leaves } = reporting({ record: record() }); + + expect(await session.leaveRequested()).toBe('proceed'); + + expect(leaves).toEqual([]); + }); + + it('reports a draft disposed with a title but an untitled slug', () => { + const onError = vi.fn(); + const { session } = sessionHarness({ + record: record({ title: 'A real title', slug: 'untitled-2' }), + onError, + }); + + session.dispose(); + session.dispose(); + + expect(onError).toHaveBeenCalledTimes(1); + expect(onError).toHaveBeenCalledWith(new Error('Draft post has title set with untitled slug'), { + extra: { slug: 'untitled-2', title: 'A real title' }, + }); + }); + + it.each([ + ['the default title', { title: DEFAULT_TITLE, slug: 'untitled' }], + ['a matching slug', { title: 'A real title', slug: 'a-real-title' }], + ['a published post', { title: 'A real title', slug: 'untitled', status: 'published' as const }], + ])('reports nothing on dispose for %s', (_label, fields) => { + const onError = vi.fn(); + const { session } = sessionHarness({ record: record(fields), onError }); + + session.dispose(); + + expect(onError).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/admin/src/editor/session/editor-session.ts b/apps/admin/src/editor/session/editor-session.ts index 7c4602e85a5..3a2ddb13524 100644 --- a/apps/admin/src/editor/session/editor-session.ts +++ b/apps/admin/src/editor/session/editor-session.ts @@ -14,11 +14,13 @@ import { type SaveCompletion, type ScheduleOptions, type SaveEngineState, + type SaveFailure, type SaveOutcome, type SaveRequest, type SaveResult, } from '@/editor/engine/save-engine'; import type { + ChangeReasonCode, EditablePostPatch, EditablePostProjection, RestoredRevision, @@ -27,6 +29,7 @@ import type { import type { LexicalInput } from '@/editor/engine/lexical-compare'; import { pick } from '@/editor/engine/pick'; import type { PostWriteOptions } from '@tryghost/admin-x-framework/api/post-contract'; +import type { EditorErrorContext } from '@/editor/report-error'; import { toSaveError } from './error-mapping'; import { createSlugPort } from './slug-port'; import { buildSaveSnapshot, type EditorSaveSnapshot } from './snapshot'; @@ -55,6 +58,20 @@ export interface EditorSaveResult extends SaveResult { post: EditorRecord; } +/** A failed request with the post it was for: its server id, if any, and its persisted status. */ +export interface EditorSaveFailure extends SaveFailure { + readonly postId: string | null; + readonly status: PostStatus; +} + +/** A leave the writer has to confirm, with why the tracker holds the post dirty. */ +export interface EditorLeaveConfirmation { + readonly postId: string | null; + readonly status: PostStatus; + readonly engineState: SaveEngineState['kind']; + readonly reasons: ChangeReasonCode[]; +} + /** Fields the engine writes onto the request rather than reading from the live post. */ const AUTHORED_KEYS = ['title', 'slug'] as const; @@ -106,7 +123,11 @@ export interface EditorSessionOptions { transport: EditorSessionTransport; /** Called once the create acknowledges; the caller replaces the URL. */ onIdAcquired: (id: string) => void; - onError: (error: unknown) => void; + onError: (error: unknown, context?: EditorErrorContext) => void; + /** Called once per request that settled as failed. */ + onSaveFailed?: (failure: EditorSaveFailure) => void; + /** Called when a leave request answers `confirm`. */ + onLeaveConfirmed?: (leave: EditorLeaveConfirmation) => void; } /** The state React renders, published together after a session change. */ @@ -223,6 +244,8 @@ export function createEditorSession({ transport, onIdAcquired, onError, + onSaveFailed, + onLeaveConfirmed, }: EditorSessionOptions): EditorSession { let identity: PersistedIdentity = record ? { id: record.id, updatedAt: record.updated_at ?? '' } @@ -614,6 +637,7 @@ export function createEditorSession({ notifyChanged(); }, onListenerError: onError, + onSaveFailed: (failure) => onSaveFailed?.({ ...failure, postId: identity.id, status }), }); // Seed the external-store snapshot before the session is handed to React. @@ -836,9 +860,38 @@ export function createEditorSession({ reauthSucceeded: () => engine.reauthSucceeded(), reauthAbandoned: () => engine.reauthAbandoned(), - leaveRequested: () => engine.leaveRequested(), + leaveRequested: async () => { + const decision = await engine.leaveRequested(); + if (decision === 'confirm' && !disposed) { + try { + onLeaveConfirmed?.({ + postId: identity.id, + status, + engineState: engine.getState().kind, + reasons: tracker.verdict().reasons.map((reason) => reason.code), + }); + } catch (error) { + onError(error); + } + } + return decision; + }, dispose: () => { + if (disposed) { + return; + } + // A draft leaving with a title but a slug still derived from the default title. + if ( + status === 'draft' && + live.slug.includes('untitled') && + live.title.trim() && + live.title !== DEFAULT_TITLE + ) { + onError(new Error('Draft post has title set with untitled slug'), { + extra: { slug: live.slug, title: live.title }, + }); + } disposed = true; pendingSlugEdits.clear(); stopSlugNotifications(); diff --git a/apps/admin/src/editor/session/session-banners.test.tsx b/apps/admin/src/editor/session/session-banners.test.tsx index f87ec9a3b19..169ca95ee06 100644 --- a/apps/admin/src/editor/session/session-banners.test.tsx +++ b/apps/admin/src/editor/session/session-banners.test.tsx @@ -1,10 +1,14 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { fireEvent, render, screen, waitFor } from '@testing-library/react'; import { editorConflictReloadConfirm } from '@tryghost/test-data/selectors/editor'; import { toast } from 'sonner'; import type { PendingSave, SaveEngineState, SaveError } from '@/editor/engine/save-engine'; +import { reportShownAlert } from '@/editor/report-error'; import { SessionBanners } from './session-banners'; import type { ReloadOutcome } from './use-editor-session'; +vi.mock('@/editor/report-error', () => ({ reportShownAlert: vi.fn() })); + const noop = () => undefined; interface BannerOverrides { @@ -29,11 +33,8 @@ function renderBanners(state: SaveEngineState, overrides: BannerOverrides = {}) ); } -const CONFLICT: SaveEngineState = { - kind: 'conflict', - intent: 'autosave', - error: { kind: 'conflict', message: 'Someone else got there first.' }, -}; +const CONFLICT_ERROR: SaveError = { kind: 'conflict', message: 'Someone else got there first.' }; +const CONFLICT: SaveEngineState = { kind: 'conflict', intent: 'autosave', error: CONFLICT_ERROR }; function errored(error: Partial): SaveEngineState { return { @@ -228,3 +229,98 @@ describe('SessionBanners', () => { }, ); }); + +describe('SessionBanners reporting', () => { + beforeEach(() => { + vi.mocked(reportShownAlert).mockClear(); + }); + + it('reports a failed-save banner once, by the text shown, not per render', () => { + const error: SaveError = { kind: 'transport', message: 'Offline' }; + const state = errored(error); + const { rerender } = renderBanners(state); + rerender( + ''} + hasUnsavedContent={() => false} + state={state} + onDismissReauth={noop} + onReload={() => Promise.resolve('reloaded')} + onRetryReauth={noop} + onRetrySave={noop} + />, + ); + + expect(reportShownAlert).toHaveBeenCalledTimes(1); + expect(reportShownAlert).toHaveBeenCalledWith( + 'Couldn’t reach the server. Your changes are still here.', + error, + ); + }); + + it('reports a collision held on a pending save once across re-renders', () => { + const blockedBy: SaveError = { kind: 'conflict', message: 'Another writer changed this post.' }; + const { rerender } = renderBanners({ kind: 'idle' }, { pendingSave: { blockedBy } }); + // The session republishes a fresh pending-save wrapper around the same error. + rerender( + ''} + hasUnsavedContent={() => false} + pendingSave={{ blockedBy }} + state={{ kind: 'idle' }} + onDismissReauth={noop} + onReload={() => Promise.resolve('reloaded')} + onRetryReauth={noop} + onRetrySave={noop} + />, + ); + + expect(reportShownAlert).toHaveBeenCalledTimes(1); + expect(reportShownAlert).toHaveBeenCalledWith(expect.any(String), blockedBy); + }); + + it('reports a collision banner with the collision behind it', () => { + renderBanners(CONFLICT); + + expect(reportShownAlert).toHaveBeenCalledTimes(1); + expect(reportShownAlert).toHaveBeenCalledWith( + expect.stringContaining('Someone else is editing this post'), + CONFLICT_ERROR, + ); + }); + + it('reports a deleted-post banner as a not-found', () => { + renderBanners({ kind: 'halted' }); + + expect(reportShownAlert).toHaveBeenCalledTimes(1); + expect(reportShownAlert).toHaveBeenCalledWith( + expect.stringContaining('This post has been deleted'), + expect.objectContaining({ kind: 'not-found' }), + ); + }); + + it('reports the deleted-post banner a reload reveals', async () => { + renderBanners(CONFLICT, { onReload: () => Promise.resolve('gone') }); + fireEvent.click(screen.getByRole('button', { name: 'Reload' })); + + await waitFor(() => expect(screen.getByRole('alert')).toHaveTextContent('has been deleted')); + + expect(reportShownAlert).toHaveBeenCalledTimes(2); + expect(vi.mocked(reportShownAlert).mock.calls[1][0]).toContain('This post has been deleted'); + }); + + it.each<[string, SaveEngineState, PendingSave | undefined]>([ + ['idle', { kind: 'idle' }, undefined], + ['saving', { kind: 'saving', intent: 'autosave' }, undefined], + ['an expired session', { kind: 'reauth-pending', intent: 'explicit' }, undefined], + [ + 'a held validation', + { kind: 'idle' }, + { blockedBy: { kind: 'validation', message: 'At least one author is required.' } }, + ], + ])('reports nothing for %s', (_label, state, pendingSave) => { + renderBanners(state, { pendingSave }); + + expect(reportShownAlert).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/admin/src/editor/session/session-banners.tsx b/apps/admin/src/editor/session/session-banners.tsx index bd33e82c493..fa0536adbf5 100644 --- a/apps/admin/src/editor/session/session-banners.tsx +++ b/apps/admin/src/editor/session/session-banners.tsx @@ -1,4 +1,4 @@ -import { useState } from 'react'; +import { useEffect, useState } from 'react'; import { toast } from 'sonner'; import { AlertDialog, @@ -21,6 +21,7 @@ import { } from '@tryghost/test-data/selectors/editor'; import type { PendingSave, SaveError, SaveEngineState } from '@/editor/engine/save-engine'; import { EDITOR_CONFIRM_DIALOG_LAYER } from '@/editor/layering'; +import { reportShownAlert } from '@/editor/report-error'; import type { ReloadOutcome } from './use-editor-session'; const SESSION_EXPIRED = 'Your session expired. Sign in again in a new tab, then retry.'; @@ -28,6 +29,8 @@ const CONFLICT = 'Someone else is editing this post. Reloading replaces what you have with their version, so copy your content first if you need it.'; const GONE = 'This post has been deleted. Copy your content and paste it into a new post to keep it.'; +// A halt carries no error of its own; the banner reports it as the not-found it is. +const NOT_FOUND: SaveError = { kind: 'not-found', message: GONE }; export interface SessionBannersProps { state: SaveEngineState; @@ -51,10 +54,20 @@ function saveErrorMessage(error: SaveError): string { } } +// Once per banner the writer reads, not per render of it. +function useShownAlert(message: string | null, error: SaveError | null): void { + useEffect(() => { + if (message !== null && error !== null) { + reportShownAlert(message, error); + } + }, [message, error]); +} + type ConflictBannerProps = Pick< SessionBannersProps, 'hasUnsavedContent' | 'contentText' | 'onReload' > & { + error: SaveError; deleted?: boolean; }; @@ -62,12 +75,14 @@ function ConflictBanner({ hasUnsavedContent, contentText, onReload, + error, deleted = false, }: ConflictBannerProps) { const [confirming, setConfirming] = useState(false); const [reloading, setReloading] = useState(false); const [reloadFoundDeleted, setReloadFoundDeleted] = useState(false); const gone = deleted || reloadFoundDeleted; + useShownAlert(gone ? GONE : CONFLICT, gone ? NOT_FOUND : error); const reload = async () => { setConfirming(false); @@ -164,6 +179,9 @@ export function SessionBanners({ onRetrySave, onReload, }: SessionBannersProps) { + const saveError = state.kind === 'error' ? state.error : null; + useShownAlert(saveError && saveErrorMessage(saveError), saveError); + if (state.kind === 'reauth-pending') { return ( diff --git a/apps/admin/src/editor/session/use-editor-session.test.tsx b/apps/admin/src/editor/session/use-editor-session.test.tsx index 4c797cb07fc..562375382f0 100644 --- a/apps/admin/src/editor/session/use-editor-session.test.tsx +++ b/apps/admin/src/editor/session/use-editor-session.test.tsx @@ -4,6 +4,7 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; import type { ReactNode } from 'react'; import { dispatchedIntents } from './__test-utils__/save-engine-spy'; import { record } from './__test-utils__/session-harness'; +import { reportLeaveConfirmation, reportSaveFailure } from '@/editor/report-error'; import type { EditorRecord } from './projection'; import { useEditorSession } from './use-editor-session'; @@ -22,6 +23,12 @@ vi.mock('@tryghost/admin-x-framework', () => ({ useLocation: () => ({ key: 'editor', state: null }), })); +vi.mock('@/editor/report-error', () => ({ + reportEditorError: vi.fn(), + reportLeaveConfirmation: vi.fn(), + reportSaveFailure: vi.fn(), +})); + // The real hooks hand back one stable function per mount; a fresh mock per // render would make the handle churn for a reason the hook does not own. const stable = vi.hoisted(() => ({ fetchApi: vi.fn(), generateSlug: vi.fn() })); @@ -163,3 +170,38 @@ describe('useEditorSession title blur', () => { ); }); }); + +describe('useEditorSession reporting', () => { + beforeEach(() => { + vi.mocked(reportSaveFailure).mockClear(); + vi.mocked(reportLeaveConfirmation).mockClear(); + }); + + it('reports a failed save for the post type the session edits', async () => { + const { result } = setup(); + act(() => result.current.bind.onTitleChange('A new title')); + + await act(() => result.current.saveExplicit()); + + expect(reportSaveFailure).toHaveBeenCalledTimes(1); + const [failure, postType] = vi.mocked(reportSaveFailure).mock.calls[0]; + expect(failure).toMatchObject({ postId: 'abc123', persisted: true, status: 'draft' }); + expect(postType).toBe('post'); + }); + + it('reports a leave the writer has to confirm', async () => { + const { result } = setup(); + act(() => result.current.bind.onTitleChange('A new title')); + await act(() => result.current.saveExplicit()); + + await act(async () => { + await expect(result.current.leaveRequested()).resolves.toBe('confirm'); + }); + + expect(reportLeaveConfirmation).toHaveBeenCalledTimes(1); + const [leave, postType] = vi.mocked(reportLeaveConfirmation).mock.calls[0]; + expect(leave).toMatchObject({ postId: 'abc123' }); + expect(leave.reasons).toContain('POST_HAS_ERROR'); + expect(postType).toBe('post'); + }); +}); diff --git a/apps/admin/src/editor/session/use-editor-session.ts b/apps/admin/src/editor/session/use-editor-session.ts index 4f5aca05af8..5fed71c2655 100644 --- a/apps/admin/src/editor/session/use-editor-session.ts +++ b/apps/admin/src/editor/session/use-editor-session.ts @@ -34,7 +34,11 @@ import { import type { RestoredRevision } from '@/editor/engine/change-tracker'; import type { LexicalInput } from '@/editor/engine/lexical-compare'; import type { PostType } from '@/editor/card-config'; -import { reportEditorError } from '@/editor/report-error'; +import { + reportEditorError, + reportLeaveConfirmation, + reportSaveFailure, +} from '@/editor/report-error'; import { contentToText } from './content-text'; import { createEditorSession, @@ -207,6 +211,8 @@ export function useEditorSession({ autosaveDebounceMs: () => autosaveDebounceMs.current, onIdAcquired: setPersistedId, onError: reportEditorError, + onSaveFailed: (failure) => reportSaveFailure(failure, postType), + onLeaveConfirmed: (leave) => reportLeaveConfirmation(leave, postType), transport: { create: async (payload: EditorCreatePayload) => { const current = transport.current; diff --git a/apps/admin/src/editor/settings/revision-preview.tsx b/apps/admin/src/editor/settings/revision-preview.tsx index b1e10db7f12..a0d874ebb5e 100644 --- a/apps/admin/src/editor/settings/revision-preview.tsx +++ b/apps/admin/src/editor/settings/revision-preview.tsx @@ -17,7 +17,7 @@ import { } from '@/settings/components/koenig-loader'; import type { PostCardConfig } from '@/editor/card-config'; import { editorFileUploader } from '@/editor/koenig-file-uploader'; -import { reportKoenigError } from '@/editor/report-error'; +import { reportKoenigError, reportKoenigRenderError } from '@/editor/report-error'; import type { RevisionEntry } from './post-history'; /** The part of Lexical's editor the loader's minimal instance type leaves out. */ @@ -95,7 +95,7 @@ export function RevisionPreview({ return (
- + diff --git a/apps/admin/src/settings/components/error-boundary.test.tsx b/apps/admin/src/settings/components/error-boundary.test.tsx new file mode 100644 index 00000000000..79a920df07e --- /dev/null +++ b/apps/admin/src/settings/components/error-boundary.test.tsx @@ -0,0 +1,54 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { render, screen } from '@testing-library/react'; +import * as Sentry from '@sentry/react'; +import ErrorBoundary from './error-boundary'; + +vi.mock('@sentry/react', () => ({ + captureException: vi.fn(), + withScope: vi.fn((callback: (scope: { setTag: () => void }) => void) => + callback({ setTag: vi.fn() }), + ), +})); + +function Exploding(): never { + throw new Error('render exploded'); +} + +beforeEach(() => { + vi.spyOn(console, 'error').mockImplementation(() => {}); +}); + +afterEach(() => { + vi.restoreAllMocks(); + vi.mocked(Sentry.captureException).mockClear(); +}); + +describe('ErrorBoundary', () => { + it('captures a render error to Sentry and shows the banner', () => { + render( + + + , + ); + + expect(screen.getByRole('alert')).toHaveTextContent('An error occurred loading the widget'); + expect(Sentry.captureException).toHaveBeenCalledWith(new Error('render exploded')); + }); + + it('hands the error to onError instead of Sentry when one is given', () => { + const onError = vi.fn(); + + render( + + + , + ); + + expect(screen.getByRole('alert')).toBeInTheDocument(); + expect(onError).toHaveBeenCalledTimes(1); + const [error, info] = onError.mock.calls[0] as [unknown, { componentStack?: string }]; + expect(error).toEqual(new Error('render exploded')); + expect(typeof info.componentStack).toBe('string'); + expect(Sentry.captureException).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/admin/src/settings/components/error-boundary.tsx b/apps/admin/src/settings/components/error-boundary.tsx index 3b93bd022d9..727d221d24d 100644 --- a/apps/admin/src/settings/components/error-boundary.tsx +++ b/apps/admin/src/settings/components/error-boundary.tsx @@ -5,6 +5,8 @@ import { Banner } from '@tryghost/shade/components'; export interface ErrorBoundaryProps { children: ReactNode; name: ReactNode; + /** Replaces the default Sentry capture; the console lines stay. */ + onError?: (error: unknown, info: ErrorInfo) => void; } /** @@ -23,10 +25,14 @@ class ErrorBoundary extends React.Component { } componentDidCatch(error: unknown, info: ErrorInfo) { - Sentry.withScope((scope) => { - scope.setTag('adminx_settings_component', info.componentStack); - Sentry.captureException(error); - }); + if (this.props.onError) { + this.props.onError(error, info); + } else { + Sentry.withScope((scope) => { + scope.setTag('adminx_settings_component', info.componentStack); + Sentry.captureException(error); + }); + } // eslint-disable-next-line no-console console.error(error); // eslint-disable-next-line no-console From 414bf1be31c65046b5284a88ae764ae1066283db Mon Sep 17 00:00:00 2001 From: Steve Larson <9larsons@gmail.com> Date: Mon, 28 Sep 2026 18:37:43 +0200 Subject: [PATCH 04/21] Fixed the React editor refusing a new post's saves while no tier is picked (#30996) no ref Adjusted the save logic such that when no tier is selected (and tier access is chosen in the sidebar), a banner shows and saves are blocked to prevent malformed data. --- ...editor-settings-access.acceptance.test.tsx | 77 ++++++++++- apps/admin/src/editor/session/README.md | 19 ++- .../src/editor/session/access-save.test.ts | 122 +++++++++++++----- .../src/editor/session/editor-session.ts | 29 ++++- .../editor/session/settings-fields.test.ts | 65 +++++++--- .../src/editor/session/settings-fields.ts | 13 +- apps/admin/src/editor/settings/README.md | 16 ++- .../src/editor/settings/access-section.tsx | 22 +++- .../editor/settings/editor-settings-port.ts | 7 + 9 files changed, 287 insertions(+), 83 deletions(-) diff --git a/apps/admin/src/editor/editor-settings-access.acceptance.test.tsx b/apps/admin/src/editor/editor-settings-access.acceptance.test.tsx index bb3776945df..770d1d4fe7e 100644 --- a/apps/admin/src/editor/editor-settings-access.acceptance.test.tsx +++ b/apps/admin/src/editor/editor-settings-access.acceptance.test.tsx @@ -3,6 +3,7 @@ import { userEvent } from 'vitest/browser'; import { browseResponse, + currentRoute, currentUserResponse, fakeAdminEndpoint, fakeEditorChrome, @@ -15,6 +16,7 @@ import { submittedPost, tier, unsavedChangesGuarded, + withFastAutosave, withoutAutosave, } from '@test-utils/acceptance'; import { editorScreen } from '@/editor/editor.screen'; @@ -142,23 +144,84 @@ describe('Post settings access', () => { await expect(editorScreen.settingsTiersError()).toHaveCount(0); }); - it('refuses the first save until an explicitly selected tier access has a tier', async () => { + it('creates a new post with specific-tier access before a tier is picked, then sends the pair', async () => { editorChrome(); - const createApi = fakeAdminEndpoint('POST', /^\/posts\/\?/, { - posts: [post({ id: NEW_POST_ID, visibility: 'public' })], + // A Public read carries every site tier, the free one included. + let created = post({ + id: NEW_POST_ID, + title: '(Untitled)', + status: 'draft', + visibility: 'public', + tiers: SITE_TIERS, + tags: [], + }); + const createApi = fakeAdminEndpoint('POST', /^\/posts\/\?/, ({ body }) => { + const submitted = (body as { posts: Partial[] }).posts[0]; + created = { ...created, ...submitted, id: NEW_POST_ID, updated_at: LOADED_AT }; + return { posts: [created] }; }); - await renderAdminApp('/editor/post', FLAG_ON); + fakeAdminEndpoint('GET', new RegExp(`^/posts/${NEW_POST_ID}/\\?`), () => ({ + posts: [created], + })); + const updateApi = fakeAdminEndpoint( + 'PUT', + new RegExp(`^/posts/${NEW_POST_ID}/\\?`), + ({ body }) => { + const submitted = (body as { posts: Partial[] }).posts[0]; + created = { + ...created, + ...submitted, + updated_at: new Date(Date.parse(created.updated_at) + 1000).toISOString(), + }; + return { posts: [created] }; + }, + ); + + await renderAdminApp('/editor/post', withFastAutosave({ labs: { editorReact: true } })); await openAccess(); await chooseVisibility('Specific tier(s)'); - await userEvent.keyboard('{Meta>}s{/Meta}'); + // The pair is staged and asks for a tier, but it does not save on its own: + // the write would carry nothing of it. await expect - .element(editorScreen.saveErrorBanner()) + .element(editorScreen.settingsTiersError()) .toHaveTextContent('Please select at least one tier.'); + await expect.element(editorScreen.settingsTier('Gold')).toBeVisible(); expect(createApi.requests).toHaveLength(0); - await expect.element(editorScreen.settingsVisibility()).toHaveTextContent('Specific tier(s)'); await expect.poll(unsavedChangesGuarded).toBe(true); + + await typeIntoBody('First words'); + + // The content creates the post with the pair left out of the write. + await expect.poll(() => createApi.requests.length, POLL).toBe(1); + expect(submittedPost(createApi)).not.toHaveProperty('visibility'); + expect(submittedPost(createApi)).not.toHaveProperty('tiers'); + await expect.poll(currentRoute, POLL).toBe(`/editor/post/${NEW_POST_ID}`); + + // The pair was never acknowledged, so the server's default visibility and + // the tiers its read carries do not replace the writer's choice. + await expect.element(editorScreen.settingsVisibility()).toHaveTextContent('Specific tier(s)'); + await expect + .element(editorScreen.settingsTiersError()) + .toHaveTextContent('Please select at least one tier.'); + await expect.element(editorScreen.settingsTiers()).toHaveAttribute('aria-invalid', 'true'); + await expect + .element(editorScreen.settingsTier('Gold')) + .toHaveAttribute('data-state', 'unchecked'); + // A save that follows the create leaves the pair out as well. + for (let index = 0; index < updateApi.requests.length; index += 1) { + expect(submittedPost(updateApi, index)).not.toHaveProperty('visibility'); + expect(submittedPost(updateApi, index)).not.toHaveProperty('tiers'); + } + + await editorScreen.settingsTier('Gold').click(); + + await expect(updateApi).toHaveSavedFields({ visibility: 'tiers', tiers: [{ id: GOLD.id }] }); + await expect(editorScreen.settingsTiersError()).toHaveCount(0); + expect(created).toMatchObject({ visibility: 'tiers', tiers: [{ id: GOLD.id }] }); + // Body typed after the create waits for the tier and lands with it. + await expect.poll(() => String(created.lexical ?? ''), POLL).toContain('First words'); }); it('sends the visibility and the tiers together once a tier is picked', async () => { diff --git a/apps/admin/src/editor/session/README.md b/apps/admin/src/editor/session/README.md index bda73022ca4..ac5c8f4ce85 100644 --- a/apps/admin/src/editor/session/README.md +++ b/apps/admin/src/editor/session/README.md @@ -74,12 +74,19 @@ command in the runnable queue, so navigating away does not wait indefinitely. The live document remains the source of truth: Update enables, the post stays dirty, and leaving requires a save or confirmation. -All saves use the same preparation validator. An incomplete tier pairing, an -over-long meta/social field, an emptied author list, or a newly staged future -publish time holds a background save with a validation blocker. Body autosave, -title and image commits follow the same rule, including an already armed timer -or queued request. The editor explains why changes are waiting even when the -settings panel is closed. A saved future publish time is not itself invalid. +All saves use the same preparation validator. An incomplete tier pairing on a +post that exists, an over-long meta/social field, an emptied author list, or a +newly staged future publish time holds a background save with a validation +blocker. Body autosave, title and image commits follow the same rule, including +an already armed timer or queued request. The editor explains why changes are +waiting even when the settings panel is closed. A saved future publish time is +not itself invalid. + +A post the server has not created yet is not held to the tier rule. Its saves +go ahead with the incomplete pair left out of the write and of the submitted +projection, so the acknowledgement is not authoritative for either field: the +pair stays the writer's edit across the create, whatever visibility and tier +relations the server answered with, and the next complete pair sends both. An explicit save returns a validation failure promptly and shows the save error. Its content stays pending; its publish/schedule/email target is not retained for diff --git a/apps/admin/src/editor/session/access-save.test.ts b/apps/admin/src/editor/session/access-save.test.ts index 202ad0e0c8f..0b9544ac263 100644 --- a/apps/admin/src/editor/session/access-save.test.ts +++ b/apps/admin/src/editor/session/access-save.test.ts @@ -4,11 +4,12 @@ import { record, serializedFields, sessionHarness, + type HarnessHooks, } from '@/editor/session/__test-utils__/session-harness'; const TIERS = [{ id: 'gold' }, { id: 'silver' }]; -function accessSession(visibility: string | null, tiers = TIERS) { +function accessSession(visibility: string | null, hooks: HarnessHooks = {}) { const saved = record({ id: 'post-id', uuid: 'post-uuid', @@ -17,7 +18,7 @@ function accessSession(visibility: string | null, tiers = TIERS) { slug: 'post', status: 'draft', visibility: visibility ?? 'paid', - tiers, + tiers: TIERS, lexical: null, updated_at: LOADED_AT, published_at: null, @@ -33,7 +34,7 @@ function accessSession(visibility: string | null, tiers = TIERS) { }, // Exercise the same serialization as the post/page transports: an unpaired // tier visibility disappears before the API sees it. - { applied: serializedFields, generateSlug: () => Promise.resolve('post') }, + { applied: serializedFields, generateSlug: () => Promise.resolve('post'), ...hooks }, ); } @@ -57,32 +58,95 @@ describe('saving post access', () => { }, ); - it.each([null, 'tiers'])( - 'refuses explicit saves and publishing with an empty tier selection (saved visibility: %s)', - async (visibility) => { - const { session, create, update } = accessSession(visibility); - session.patchFields({ visibility: 'tiers', tiers: [] }); - session.commitField(); - - for (const save of [session.dispatchExplicit, session.dispatchPublish]) { - expect(await save()).toMatchObject({ - kind: 'failed', - error: { kind: 'validation', message: 'Please select at least one tier.' }, - }); - } - expect(create).not.toHaveBeenCalled(); - expect(update).not.toHaveBeenCalled(); - expect(session.getFields()).toMatchObject({ visibility: 'tiers', tiers: [] }); - expect(session.hasUnsavedContent()).toBe(true); - - // Correcting the selection must recover from the validation failure. - session.patchFields({ tiers: [TIERS[0]] }); - expect(await session.dispatchExplicit()).toMatchObject({ kind: 'saved' }); - expect(session.getFields()).toMatchObject({ visibility: 'tiers', tiers: [TIERS[0]] }); - expect(session.isDirty()).toBe(false); - session.dispose(); - }, - ); + it('refuses explicit saves and publishing with an empty tier selection on a saved post', async () => { + const { session, update } = accessSession('tiers'); + session.patchFields({ visibility: 'tiers', tiers: [] }); + session.commitField(); + + for (const save of [session.dispatchExplicit, session.dispatchPublish]) { + expect(await save()).toMatchObject({ + kind: 'failed', + error: { kind: 'validation', message: 'Please select at least one tier.' }, + }); + } + expect(update).not.toHaveBeenCalled(); + expect(session.getFields()).toMatchObject({ visibility: 'tiers', tiers: [] }); + expect(session.hasUnsavedContent()).toBe(true); + + // Correcting the selection must recover from the validation failure. + session.patchFields({ tiers: [TIERS[0]] }); + expect(await session.dispatchExplicit()).toMatchObject({ kind: 'saved' }); + expect(session.getFields()).toMatchObject({ visibility: 'tiers', tiers: [TIERS[0]] }); + expect(session.isDirty()).toBe(false); + session.dispose(); + }); + + it('creates a new post with an empty tier selection and keeps the pair as the writer’s edit', async () => { + const { session, state, create, update } = accessSession(null); + session.patchFields({ visibility: 'tiers', tiers: [] }); + + expect(await session.dispatchExplicit()).toMatchObject({ kind: 'saved' }); + expect(create).toHaveBeenCalledTimes(1); + expect(create.mock.calls[0][0]).not.toHaveProperty('visibility'); + expect(create.mock.calls[0][0]).not.toHaveProperty('tiers'); + expect(state.acquiredIds).toEqual(['post-id']); + // The server answered with its default and the tiers a read carries; the + // pair was never sent, so neither replaces the writer's choice. + expect(session.getFields()).toMatchObject({ visibility: 'tiers', tiers: [] }); + expect(session.isDirty()).toBe(true); + + // The post exists now, so the same pair holds the next save. + expect(await session.dispatchExplicit()).toMatchObject({ + kind: 'failed', + error: { kind: 'validation', message: 'Please select at least one tier.' }, + }); + expect(update).not.toHaveBeenCalled(); + + session.patchFields({ tiers: [TIERS[0]] }); + expect(await session.dispatchExplicit()).toMatchObject({ kind: 'saved' }); + expect(update.mock.calls[0][0]).toMatchObject({ visibility: 'tiers', tiers: [TIERS[0]] }); + expect(session.getFields()).toMatchObject({ visibility: 'tiers', tiers: [TIERS[0]] }); + expect(session.isDirty()).toBe(false); + session.dispose(); + }); + + it('refuses to publish a new post with an empty tier selection', async () => { + const { session, create } = accessSession(null); + session.patchFields({ visibility: 'tiers', tiers: [] }); + + expect(await session.dispatchPublish()).toMatchObject({ + kind: 'failed', + error: { kind: 'validation', message: 'Please select at least one tier.' }, + }); + expect(create).not.toHaveBeenCalled(); + + // A draft create leaves the pair out instead. + expect(await session.dispatchExplicit()).toMatchObject({ kind: 'saved' }); + expect(create).toHaveBeenCalledTimes(1); + expect(create.mock.calls[0][0]).not.toHaveProperty('visibility'); + session.dispose(); + }); + + it('sends a tier picked while the create is in flight together with the visibility', async () => { + const harness = accessSession(null, { + duringSave: () => harness.session.patchFields({ visibility: 'tiers', tiers: [TIERS[0]] }), + }); + const { session, create, update } = harness; + session.patchFields({ visibility: 'tiers', tiers: [] }); + + expect(await session.dispatchExplicit()).toMatchObject({ kind: 'saved' }); + expect(create.mock.calls[0][0]).not.toHaveProperty('visibility'); + expect(create.mock.calls[0][0]).not.toHaveProperty('tiers'); + // The visibility was never sent, so the acknowledgement does not move it + // off the tier the writer picked in the meantime. + expect(session.getFields()).toMatchObject({ visibility: 'tiers', tiers: [TIERS[0]] }); + expect(session.isDirty()).toBe(true); + + expect(await session.dispatchExplicit()).toMatchObject({ kind: 'saved' }); + expect(update.mock.calls[0][0]).toMatchObject({ visibility: 'tiers', tiers: [TIERS[0]] }); + expect(session.isDirty()).toBe(false); + session.dispose(); + }); it('lets the server apply untouched access defaults on create', async () => { const { session, create } = accessSession(null); diff --git a/apps/admin/src/editor/session/editor-session.ts b/apps/admin/src/editor/session/editor-session.ts index 3a2ddb13524..b6b94b47939 100644 --- a/apps/admin/src/editor/session/editor-session.ts +++ b/apps/admin/src/editor/session/editor-session.ts @@ -41,6 +41,7 @@ import { identityFor, publishedAtInFuture, settingsFieldError, + tiersIncomplete, validatedFieldsOf, type EditorSettingsPatch, type EditorSettingsFields, @@ -438,12 +439,13 @@ export function createEditorSession({ const stopSlugNotifications = machine.subscribe(notifyChanged); // The post validator runs before every save: an explicit tier selection needs a - // tier even on the first save, and an over-long field is not sent. + // tier once the post exists or leaves draft, and an over-long field is not sent. function requestInvalid( request: SaveRequest, projection: EditablePostPatch, ): string | null { - const invalid = settingsFieldError(validatedFieldsOf(live)); + const creatingDraft = request.snapshot.id === null && request.target.status === 'draft'; + const invalid = settingsFieldError(validatedFieldsOf(live), creatingDraft); if (invalid) { return invalid; } @@ -495,10 +497,17 @@ export function createEditorSession({ stageSettingsField(key, live, projection, payload); } } - // The write contract requires the pair even when only one field changed. - // Reads include tier relations for Public and Paid posts too, so switching - // to specific tiers can leave the relation IDs unchanged. - if (live.visibility === 'tiers' && ('visibility' in payload || 'tiers' in payload)) { + if (tiersIncomplete(live)) { + // The transport drops the unpaired pair (post-contract.ts). Kept out of the + // submitted projection too, or the ack rebases the held visibility away. + delete projection.visibility; + delete payload.visibility; + delete projection.tiers; + delete payload.tiers; + } else if (live.visibility === 'tiers' && ('visibility' in payload || 'tiers' in payload)) { + // The write contract requires the pair even when only one field changed. + // Reads include tier relations for Public and Paid posts too, so switching + // to specific tiers can leave the relation IDs unchanged. projection.visibility = live.visibility; payload.visibility = live.visibility; projection.tiers = live.tiers; @@ -585,13 +594,19 @@ export function createEditorSession({ // A matching refetch can make an unsubmitted edit look saved. Preserve // those edits through the rebase, whose fallback base is the latest saved // copy. Submitted fields already have a stable base in the request. - const unsubmittedEdits = Object.fromEntries( + const unsubmittedEdits: EditablePostPatch = Object.fromEntries( SETTINGS_FIELD_KEYS.filter( (key) => prepared.projection[key] === undefined && (writerEdits.get(key) ?? 0) > prepared.builtAtVersion, ).map((key) => [key, live[key]]), ); + // A pair left out of the write was never acknowledged; its empty tier list + // equals a new post's saved one, so the rebase would take the server's relations. + if (prepared.projection.visibility === undefined && tiersIncomplete(live)) { + unsubmittedEdits.visibility = live.visibility; + unsubmittedEdits.tiers = live.tiers; + } const acknowledged = projectionOf(result.post); tracker.saveAcknowledged(result.id, prepared.projection, acknowledged); tracker.setLive(result.id, unsubmittedEdits); diff --git a/apps/admin/src/editor/session/settings-fields.test.ts b/apps/admin/src/editor/session/settings-fields.test.ts index c8f889c2fee..4a6e60ceb5d 100644 --- a/apps/admin/src/editor/session/settings-fields.test.ts +++ b/apps/admin/src/editor/session/settings-fields.test.ts @@ -17,6 +17,7 @@ import { overLength, settingsFieldError, settingsFieldErrorFor, + tiersIncomplete, validatedFieldsOf, type ValidatedSettingsFieldKey, type ValidatedSettingsFields, @@ -59,65 +60,87 @@ describe('validatedFieldsOf', () => { const withoutMetaTitle = { ...validated }; delete (withoutMetaTitle as Partial).meta_title; - expect(settingsFieldError(validated)).toBe(META_TITLE_TOO_LONG); - expect(settingsFieldError(withoutMetaTitle)).toBeNull(); + expect(settingsFieldError(validated, false)).toBe(META_TITLE_TOO_LONG); + expect(settingsFieldError(withoutMetaTitle, false)).toBeNull(); }); }); describe('settingsFieldError', () => { it('passes fields that break no rule', () => { - expect(settingsFieldError(VALID)).toBeNull(); + expect(settingsFieldError(VALID, false)).toBeNull(); }); - it('refuses specific-tier access without a tier', () => { - expect(settingsFieldError({ ...VALID, visibility: 'tiers' })).toBe(TIERS_REQUIRED); + it('refuses specific-tier access without a tier on a post that exists', () => { + expect(settingsFieldError({ ...VALID, visibility: 'tiers' }, false)).toBe(TIERS_REQUIRED); }); - it('refuses a meta title past the column width', () => { - expect(settingsFieldError({ ...VALID, meta_title: 'a'.repeat(META_TITLE_MAX) })).toBeNull(); - expect(settingsFieldError({ ...VALID, meta_title: 'a'.repeat(META_TITLE_MAX + 1) })).toBe( + it('lets a new post keep specific-tier access without a tier', () => { + expect(settingsFieldError({ ...VALID, visibility: 'tiers' }, true)).toBeNull(); + // The pair is still incomplete: the section asks for a tier either way. + expect(tiersIncomplete({ ...VALID, visibility: 'tiers' })).toBe(true); + }); + + it('holds a new post to every other rule', () => { + expect(settingsFieldError({ ...VALID, meta_title: 'a'.repeat(META_TITLE_MAX + 1) }, true)).toBe( META_TITLE_TOO_LONG, ); }); + it('refuses a meta title past the column width', () => { + expect( + settingsFieldError({ ...VALID, meta_title: 'a'.repeat(META_TITLE_MAX) }, false), + ).toBeNull(); + expect( + settingsFieldError({ ...VALID, meta_title: 'a'.repeat(META_TITLE_MAX + 1) }, false), + ).toBe(META_TITLE_TOO_LONG); + }); + it('refuses a meta description past the column width', () => { expect( - settingsFieldError({ ...VALID, meta_description: 'a'.repeat(META_DESCRIPTION_MAX) }), + settingsFieldError({ ...VALID, meta_description: 'a'.repeat(META_DESCRIPTION_MAX) }, false), ).toBeNull(); expect( - settingsFieldError({ ...VALID, meta_description: 'a'.repeat(META_DESCRIPTION_MAX + 1) }), + settingsFieldError( + { ...VALID, meta_description: 'a'.repeat(META_DESCRIPTION_MAX + 1) }, + false, + ), ).toBe(META_DESCRIPTION_TOO_LONG); }); it('refuses a Facebook title past the column width', () => { - expect(settingsFieldError({ ...VALID, og_title: 'a'.repeat(OG_TITLE_MAX) })).toBeNull(); - expect(settingsFieldError({ ...VALID, og_title: 'a'.repeat(OG_TITLE_MAX + 1) })).toBe( + expect(settingsFieldError({ ...VALID, og_title: 'a'.repeat(OG_TITLE_MAX) }, false)).toBeNull(); + expect(settingsFieldError({ ...VALID, og_title: 'a'.repeat(OG_TITLE_MAX + 1) }, false)).toBe( OG_TITLE_TOO_LONG, ); }); it('refuses a Facebook description past the column width', () => { expect( - settingsFieldError({ ...VALID, og_description: 'a'.repeat(OG_DESCRIPTION_MAX) }), + settingsFieldError({ ...VALID, og_description: 'a'.repeat(OG_DESCRIPTION_MAX) }, false), ).toBeNull(); expect( - settingsFieldError({ ...VALID, og_description: 'a'.repeat(OG_DESCRIPTION_MAX + 1) }), + settingsFieldError({ ...VALID, og_description: 'a'.repeat(OG_DESCRIPTION_MAX + 1) }, false), ).toBe(OG_DESCRIPTION_TOO_LONG); }); it('refuses an X title past the column width', () => { - expect(settingsFieldError({ ...VALID, twitter_title: 'a'.repeat(X_TITLE_MAX) })).toBeNull(); - expect(settingsFieldError({ ...VALID, twitter_title: 'a'.repeat(X_TITLE_MAX + 1) })).toBe( - X_TITLE_TOO_LONG, - ); + expect( + settingsFieldError({ ...VALID, twitter_title: 'a'.repeat(X_TITLE_MAX) }, false), + ).toBeNull(); + expect( + settingsFieldError({ ...VALID, twitter_title: 'a'.repeat(X_TITLE_MAX + 1) }, false), + ).toBe(X_TITLE_TOO_LONG); }); it('refuses an X description past the column width', () => { expect( - settingsFieldError({ ...VALID, twitter_description: 'a'.repeat(X_DESCRIPTION_MAX) }), + settingsFieldError({ ...VALID, twitter_description: 'a'.repeat(X_DESCRIPTION_MAX) }, false), ).toBeNull(); expect( - settingsFieldError({ ...VALID, twitter_description: 'a'.repeat(X_DESCRIPTION_MAX + 1) }), + settingsFieldError( + { ...VALID, twitter_description: 'a'.repeat(X_DESCRIPTION_MAX + 1) }, + false, + ), ).toBe(X_DESCRIPTION_TOO_LONG); }); @@ -173,7 +196,7 @@ describe('settingsFieldErrorFor', () => { for (const [key, fields] of overLimit) { expect(settingsFieldErrorFor(key, fields)).not.toBeNull(); - expect(settingsFieldErrorFor(key, fields)).toBe(settingsFieldError(fields)); + expect(settingsFieldErrorFor(key, fields)).toBe(settingsFieldError(fields, false)); } }); }); diff --git a/apps/admin/src/editor/session/settings-fields.ts b/apps/admin/src/editor/session/settings-fields.ts index 2bc664ea8a7..bd4bf1f2d0d 100644 --- a/apps/admin/src/editor/session/settings-fields.ts +++ b/apps/admin/src/editor/session/settings-fields.ts @@ -93,7 +93,7 @@ export const OG_DESCRIPTION_TOO_LONG = `Facebook description cannot be longer th export const X_TITLE_TOO_LONG = `X title cannot be longer than ${X_TITLE_MAX} characters.`; export const X_DESCRIPTION_TOO_LONG = `X description cannot be longer than ${X_DESCRIPTION_MAX} characters.`; -/** `visibility: 'tiers'` with no tiers: the write contract drops the visibility. */ +/** `visibility: 'tiers'` with no tiers: the write contract drops the pair. */ export function tiersIncomplete( fields: Pick, ): boolean { @@ -172,9 +172,16 @@ export function settingsFieldErrorFor( return overLength(fields[key], max) ? message : null; } -/** The first rule the settings fields break, in the post validator's order. */ -export function settingsFieldError(fields: ValidatedSettingsFields): string | null { +/** + * The first rule the settings fields break, in the post validator's order. A + * post the server has not created yet is not held to the tier rule + * (validators/post.js `isNew`); its write leaves the pair out instead. + */ +export function settingsFieldError(fields: ValidatedSettingsFields, isNew: boolean): string | null { for (const key of VALIDATED_SETTINGS_FIELD_KEYS) { + if (isNew && key === 'tiers') { + continue; + } const error = settingsFieldErrorFor(key, fields); if (error) { return error; diff --git a/apps/admin/src/editor/settings/README.md b/apps/admin/src/editor/settings/README.md index f58280d11d6..bcbc4201e69 100644 --- a/apps/admin/src/editor/settings/README.md +++ b/apps/admin/src/editor/settings/README.md @@ -191,12 +191,16 @@ selection, and a tier ID without type metadata is preserved. A failed tier lookup shows an error and a Retry action in place of the list. An empty tier selection is staged like any other edit but never sent: the -section asks for at least one tier, and while the pairing is incomplete no field -save runs and a save the writer asks for is refused with the same message. -Because the pairing is staged rather than held in the panel, it survives closing -the sidebar, enables Update and is what the leave guard asks about. A create -with untouched access settings still uses the server default; an explicit tier -selection must include a tier even on the first save. +section asks for at least one tier. On a post that exists, no field save runs +while the pairing is incomplete and a save the writer asks for is refused with +the same message. Because the pairing is staged rather than held in the panel, +it survives closing the sidebar, enables Update and is what the leave guard asks +about. A post the server has not created yet is not held to the rule: the +section stages the incomplete pair without a save of its own, content saves go +ahead with the pair left out, and the pair stays the writer's edit across the +create, so the section keeps asking for a tier and the first tier picked sends +visibility and tiers together. A create with untouched access settings uses the +server default. When either access field changes to specific tiers, the save submits both visibility and the tier list, including tier IDs the writer never touched. A diff --git a/apps/admin/src/editor/settings/access-section.tsx b/apps/admin/src/editor/settings/access-section.tsx index c50287c9536..e2f419f92e6 100644 --- a/apps/admin/src/editor/settings/access-section.tsx +++ b/apps/admin/src/editor/settings/access-section.tsx @@ -21,8 +21,12 @@ import type { PostType } from '@/editor/card-config'; import { PAID_TIERS_SEARCH_PARAMS } from '@/editor/browse-params'; import { useEditorSettings } from '@/editor/use-editor-settings'; import { EDITOR_REQUEST_OPTIONS } from '@/editor/request-options'; -import { TIERS_REQUIRED, tiersIncomplete } from '@/editor/session/settings-fields'; -import type { EditorSettingsPort } from './editor-settings-port'; +import { + TIERS_REQUIRED, + tiersIncomplete, + type EditorSettingsFields, +} from '@/editor/session/settings-fields'; +import { type EditorSettingsPort, isNewPost } from './editor-settings-port'; import { SectionLoadError } from './section-load-error'; import { SettingsSection } from './settings-section'; import { @@ -132,9 +136,19 @@ export function AccessSection({ session, postType }: AccessSectionProps) { }, [fetchNextPage, hasNextPage, isFetchingNextPage, tiersFailed]); const options = hasNextPage ? [] : tierOptions(tiersData?.tiers); + // An incomplete pair is left out of every write. On a post the server has not + // created yet it is staged without a save of its own: that write would carry nothing. + const editAccess = (patch: Pick) => { + if (tiersIncomplete(patch) && isNewPost(session)) { + session.stageSettings(patch); + return; + } + session.editSettings(patch); + }; + // Leaving `tiers` clears the tiers it granted, as the tier pickers do. const changeVisibility = (next: string) => - session.editSettings({ + editAccess({ visibility: next, tiers: next === 'tiers' ? postTiers(session.settings.tiers) : [], }); @@ -144,7 +158,7 @@ export function AccessSection({ session, postType }: AccessSectionProps) { if (!next.delete(id)) { next.add(id); } - session.editSettings({ visibility: 'tiers', tiers: tiersFromSelection(options, next) }); + editAccess({ visibility: 'tiers', tiers: tiersFromSelection(options, next) }); }; return ( diff --git a/apps/admin/src/editor/settings/editor-settings-port.ts b/apps/admin/src/editor/settings/editor-settings-port.ts index 3a014a286b7..754d7333089 100644 --- a/apps/admin/src/editor/settings/editor-settings-port.ts +++ b/apps/admin/src/editor/settings/editor-settings-port.ts @@ -23,6 +23,13 @@ export type EditorSettingsPort = Pick< bind: Pick; }; +/** A post the server has not created yet: nothing loaded and no id acquired. */ +export function isNewPost( + session: Pick, +): boolean { + return !session.loadedRecord && session.createdId === null; +} + /** The port's identity tracks the members it carries, not the render that produced it. */ export function useEditorSettingsPort(session: EditorSessionHandle): EditorSettingsPort { const { title, excerpt, onExcerptChange } = session.bind; From 4669ca5a76b8d138d96fa6b07c7d3070574ed012 Mon Sep 17 00:00:00 2001 From: Austin Burdine Date: Mon, 28 Sep 2026 12:45:35 -0400 Subject: [PATCH 05/21] Added in-development table support for iterating on new schemas (#30987) no ref A new table's shape usually changes several times before its feature ships. Today every change needs its own versioned migration and schema hash bump, so tables pick up a trail of migrations that only existed to reshape them during development. Tables listed in schema/in-development.ts are defined only in schema.js until they are final. knex-migrator init creates them only where the new createInDevelopmentTables config is enabled (development and testing), boot creates them in an existing dev database if missing, and `pnpm migrate:rebuild-in-development-tables` drops and recreates them to pick up definition changes. They're left out of the integrity hash, and finalised tables may not reference them. Once final, the table is removed from the list and gets its versioned migration as usual. --- .../skills/create-database-migration/SKILL.md | 4 + docs/practices/database-migrations.md | 30 +++ .../core/bin/rebuild-in-development-tables.ts | 23 +++ .../server/data/db/database-state-manager.js | 9 + .../data/migrations/init/1-create-tables.js | 8 +- .../server/data/migrations/utils/tables.js | 76 +++++++- ghost/core/core/server/data/schema/README.md | 5 + .../core/server/data/schema/in-development.ts | 107 +++++++++++ ghost/core/core/server/data/schema/index.js | 1 + ghost/core/core/shared/config/defaults.json | 1 + .../shared/config/env/config.development.json | 1 + .../config/env/config.testing-mysql.json | 1 + .../shared/config/env/config.testing.json | 1 + ghost/core/package.json | 1 + .../migrations/in-development-tables.test.ts | 174 ++++++++++++++++++ .../server/data/schema/in-development.test.ts | 98 ++++++++++ .../unit/server/data/schema/integrity.test.js | 3 +- 17 files changed, 537 insertions(+), 6 deletions(-) create mode 100644 ghost/core/bin/rebuild-in-development-tables.ts create mode 100644 ghost/core/core/server/data/schema/in-development.ts create mode 100644 ghost/core/test/integration/migrations/in-development-tables.test.ts create mode 100644 ghost/core/test/unit/server/data/schema/in-development.test.ts diff --git a/.agents/skills/create-database-migration/SKILL.md b/.agents/skills/create-database-migration/SKILL.md index e6bd516223f..ae3fd91d90f 100644 --- a/.agents/skills/create-database-migration/SKILL.md +++ b/.agents/skills/create-database-migration/SKILL.md @@ -12,6 +12,10 @@ accompanies it. ## Instructions +If you are adding a new table whose shape is still changing, consider marking it +as in development instead of writing a migration yet; see "New tables still in +development" in the guide. + 1. Create a new, empty migration file: `cd ghost/core && pnpm migrate:create `. IMPORTANT: do not create the migration file manually; always use this script to create the initial empty migration file. The slug must be kebab-case (e.g. `add-column-to-posts`). 2. The above command will create a new directory in `ghost/core/core/server/data/migrations/versions` if needed, create the empty migration file with the appropriate name, and bump the core and admin package versions to RC if this is the first migration after a release. 3. Update the migration file with the changes you want to make in the database, following the existing patterns in the codebase. Where appropriate, prefer to use the utility functions in `ghost/core/core/server/data/migrations/utils/*`. diff --git a/docs/practices/database-migrations.md b/docs/practices/database-migrations.md index a7a008d6750..fd2a537c471 100644 --- a/docs/practices/database-migrations.md +++ b/docs/practices/database-migrations.md @@ -84,6 +84,36 @@ pnpm knex-migrator rollback --v --force method. You can use this workflow to iterate while `down()` restores the same state that existed before `up()`. +### New tables still in development + +A new table's shape often changes several times before its feature is ready. +Rather than writing a migration for each change, list the table in +`IN_DEVELOPMENT_TABLES` in +[`core/server/data/schema/in-development.ts`](../../ghost/core/core/server/data/schema/in-development.ts) +and define its schema in `schema.js` without adding a versioned migration. It +is only created in development and testing databases. + +To apply a changed definition to your local database, rebuild the listed +tables. This drops them and discards their data: + +```bash +cd ghost/core +pnpm migrate:rebuild-in-development-tables +``` + +Code that reads or writes the table must stay dormant wherever the table is not +created, for example behind a [feature flag](feature-flags.md). +The table still needs a classification in the exporter table lists. + +When the definition is final, remove the table from `IN_DEVELOPMENT_TABLES`, +add the versioned migration that creates it, and update the schema integrity +hash. Create the table with +`addTable(name, tableSpec, {replaceDevelopmentCopy: true})`: development and +testing databases already have a copy built from an earlier definition, and the +option replaces it with the final one. This discards its data and drops the +tables that reference it; Ghost recreates the ones still in development when it +next boots. From then on the table follows the normal migration rules. + ### Testing The database-backed migration suites run against MySQL, Ghost's supported diff --git a/ghost/core/bin/rebuild-in-development-tables.ts b/ghost/core/bin/rebuild-in-development-tables.ts new file mode 100644 index 00000000000..e641598e34f --- /dev/null +++ b/ghost/core/bin/rebuild-in-development-tables.ts @@ -0,0 +1,23 @@ +#!/usr/bin/env node + +// Drops and recreates the in-development tables listed in +// core/server/data/schema/in-development.ts so a local database picks up +// changes to their definitions. Their data is discarded. + +import '../core/server/overrides'; +import logging from '@tryghost/logging'; +import db from '../core/server/data/db'; +import { rebuildInDevelopmentTables } from '../core/server/data/schema/in-development'; + +async function main() { + try { + await rebuildInDevelopmentTables(db.knex); + } catch (err) { + logging.error(err); + process.exitCode = 1; + } finally { + await db.knex.destroy(); + } +} + +main(); diff --git a/ghost/core/core/server/data/db/database-state-manager.js b/ghost/core/core/server/data/db/database-state-manager.js index a98e1caa4f4..0905b9e76e0 100644 --- a/ghost/core/core/server/data/db/database-state-manager.js +++ b/ghost/core/core/server/data/db/database-state-manager.js @@ -30,6 +30,12 @@ const printState = ({ state }) => { } }; +const createMissingInDevelopmentTables = async () => { + const { inDevelopment } = require('../schema'); + const db = require('./index'); + await inDevelopment.createMissingInDevelopmentTables(db.knex); +}; + class DatabaseStateManager { constructor({ knexMigratorFilePath }) { this.knexMigrator = new KnexMigrator({ @@ -83,6 +89,7 @@ class DatabaseStateManager { printState({ state }); if (state === states.READY) { + await createMissingInDevelopmentTables(); return; } @@ -111,6 +118,8 @@ class DatabaseStateManager { state = await this.getState(); printState({ state }); + + await createMissingInDevelopmentTables(); } catch (error) { let errorToThrow = error; if (!errors.utils.isGhostError(error)) { diff --git a/ghost/core/core/server/data/migrations/init/1-create-tables.js b/ghost/core/core/server/data/migrations/init/1-create-tables.js index 2ae6f5e40e1..18b8a7c9e14 100644 --- a/ghost/core/core/server/data/migrations/init/1-create-tables.js +++ b/ghost/core/core/server/data/migrations/init/1-create-tables.js @@ -1,14 +1,16 @@ const commands = require('../../schema').commands; -const schema = require('../../schema').tables; const views = require('../../schema').views; +const inDevelopment = require('../../schema').inDevelopment; const logging = require('@tryghost/logging'); -const schemaTables = Object.keys(schema); module.exports.up = async (options) => { const connection = options.connection; const existingTables = await commands.getTables(connection); - const missingTables = schemaTables.filter((t) => !existingTables.includes(t)); + // In-development tables are only created where config enables them, see + // schema/in-development.ts + const tablesToCreate = inDevelopment.getTablesToCreate(); + const missingTables = tablesToCreate.filter((t) => !existingTables.includes(t)); for (const table of missingTables) { logging.info('Creating table: ' + table); diff --git a/ghost/core/core/server/data/migrations/utils/tables.js b/ghost/core/core/server/data/migrations/utils/tables.js index 348c2749a11..c855eb73914 100644 --- a/ghost/core/core/server/data/migrations/utils/tables.js +++ b/ghost/core/core/server/data/migrations/utils/tables.js @@ -1,19 +1,87 @@ const logging = require('@tryghost/logging'); +const DatabaseInfo = require('@tryghost/database-info'); +const config = require('../../../../shared/config'); const { commands } = require('../../schema'); const { createIrreversibleMigration, createNonTransactionalMigration } = require('./migrations'); +function isDevelopmentOrTesting() { + const env = config.get('env'); + return env === 'development' || env.startsWith('testing'); +} + +/** + * @param {import('knex').Knex} connection + * @param {string} table + * @returns {Promise} the other tables with a foreign key to `table` + */ +async function getReferencingTables(connection, table) { + if (DatabaseInfo.isMySQL(connection)) { + const [rows] = await connection.raw( + `SELECT DISTINCT TABLE_NAME AS name + FROM information_schema.KEY_COLUMN_USAGE + WHERE REFERENCED_TABLE_SCHEMA = DATABASE() + AND REFERENCED_TABLE_NAME = ? + AND TABLE_NAME <> ?`, + [table, table], + ); + return rows.map((/** @type {{name: string}} */ row) => row.name); + } + + const referencing = []; + for (const other of await commands.getTables(connection)) { + if (other === table) { + continue; + } + const foreignKeys = await connection.raw(`PRAGMA foreign_key_list('${other}');`); + if ( + foreignKeys.some((/** @type {{table: string}} */ foreignKey) => foreignKey.table === table) + ) { + referencing.push(other); + } + } + return referencing; +} + +/** + * Drops a development copy of a table along with the tables that reference it, + * so no rows are left pointing at a table that no longer exists. Boot recreates + * the dropped tables that are still in development. + * + * @param {import('knex').Knex} connection + * @param {string} table + * @param {Set} [dropped] + */ +async function dropDevelopmentCopy(connection, table, dropped = new Set()) { + dropped.add(table); + for (const referencing of await getReferencingTables(connection, table)) { + if (!dropped.has(referencing)) { + await dropDevelopmentCopy(connection, referencing, dropped); + } + } + + logging.info(`Dropping development copy of table: ${table}`); + await commands.deleteTable(table, connection); +} + /** * Creates a migrations which will add a new table from schema.js to the database * @param {string} name - table name * @param {Object} tableSpec - copy of table schema definition as defined in schema.js at the moment of writing the migration, this parameter MUST be present + * @param {Object} [options] + * @param {boolean} [options.replaceDevelopmentCopy] - set when finalising a table that was in development (see + * schema/in-development.ts). Development and testing databases already have a copy built from an earlier + * definition, so there an existing table is replaced, discarding its data and dropping the tables that reference + * it. Other environments skip it as usual. * * @returns {Object} migration object returning config/up/down properties */ -function addTable(name, tableSpec) { +function addTable(name, tableSpec, { replaceDevelopmentCopy = false } = {}) { return createNonTransactionalMigration( async function up(connection) { const tableExists = await connection.schema.hasTable(name); - if (tableExists) { + if (tableExists && replaceDevelopmentCopy && isDevelopmentOrTesting()) { + await dropDevelopmentCopy(connection, name); + } else if (tableExists) { logging.warn(`Skipping adding table: ${name} - table already exists`); return; } @@ -28,6 +96,10 @@ function addTable(name, tableSpec) { return; } + if (replaceDevelopmentCopy && isDevelopmentOrTesting()) { + return dropDevelopmentCopy(connection, name); + } + logging.info(`Dropping table: ${name}`); return commands.deleteTable(name, connection); }, diff --git a/ghost/core/core/server/data/schema/README.md b/ghost/core/core/server/data/schema/README.md index 03a8b11ca60..abd3f28ad21 100644 --- a/ghost/core/core/server/data/schema/README.md +++ b/ghost/core/core/server/data/schema/README.md @@ -85,3 +85,8 @@ Changing `schema.js` alone does not update an existing database. Every schema change needs a migration that moves an installed database to the new shape. Follow the [database migrations guide](../../../../../../docs/practices/database-migrations.md) for generation, iteration, testing, and review requirements. + +Tables listed in `in-development.ts` are the exception: they are created only +in development and testing databases and need no migration until their +definition is final. See the guide's section on new tables still in +development. diff --git a/ghost/core/core/server/data/schema/in-development.ts b/ghost/core/core/server/data/schema/in-development.ts new file mode 100644 index 00000000000..ff0aaceb597 --- /dev/null +++ b/ghost/core/core/server/data/schema/in-development.ts @@ -0,0 +1,107 @@ +import logging from '@tryghost/logging'; +import type { Knex } from 'knex'; +import config from '../../../shared/config'; +// @ts-expect-error This module lacks type definitions. +import commands from './commands'; +// @ts-expect-error This module lacks type definitions. +import schema from './schema'; + +/** + * Tables listed here are defined in `schema.js` but are still being iterated on. + * + * - `knex-migrator init` only creates them in the development and testing + * environments, and only while `createInDevelopmentTables` is enabled in + * config, so production databases never contain them. + * - Boot creates any that are missing from an existing development database. + * - They need no versioned migration and are left out of the schema integrity + * hash, so their definition can change freely. + * + * Once a table's definition is final, remove it from this list and add the + * versioned migration that creates it. + * + * Code that reads or writes these tables must stay dormant wherever the tables + * are not created. + */ +export const IN_DEVELOPMENT_TABLES: string[] = []; + +export function isInDevelopmentTable(tableName: string): boolean { + return IN_DEVELOPMENT_TABLES.includes(tableName); +} + +/** + * Only development and testing databases may contain in-development tables, + * whatever the config says, so a misconfigured production site can't create + * tables that have no migrations + */ +export function shouldCreateInDevelopmentTables(): boolean { + const env: string = config.get('env'); + const isDevelopmentOrTesting = env === 'development' || env.startsWith('testing'); + return isDevelopmentOrTesting && config.get('createInDevelopmentTables') === true; +} + +/** + * The in-development tables, in the order they appear in `schema.js` + */ +export function getInDevelopmentTables(): string[] { + return Object.keys(schema).filter(isInDevelopmentTable); +} + +/** + * The tables `knex-migrator init` should create in the current environment + */ +export function getTablesToCreate(): string[] { + const includeInDevelopment = shouldCreateInDevelopmentTables(); + return Object.keys(schema).filter( + (tableName) => includeInDevelopment || !isInDevelopmentTable(tableName), + ); +} + +/** + * Creates in-development tables missing from an already initialised database + */ +export async function createMissingInDevelopmentTables(knex: Knex): Promise { + if (!shouldCreateInDevelopmentTables()) { + return; + } + + const tables = getInDevelopmentTables(); + if (!tables.length) { + return; + } + + const existingTables: string[] = await commands.getTables(knex); + + for (const tableName of tables) { + if (!existingTables.includes(tableName)) { + logging.info(`Creating in-development table: ${tableName}`); + await commands.createTable(tableName, knex); + } + } +} + +/** + * Drops and recreates every in-development table, discarding its data, so the + * database picks up changes to their definitions in `schema.js` + */ +export async function rebuildInDevelopmentTables(knex: Knex): Promise { + if (!shouldCreateInDevelopmentTables()) { + logging.warn( + 'In-development tables are disabled in this environment (createInDevelopmentTables)', + ); + return; + } + + const tables = getInDevelopmentTables(); + + // schema.js lists each table after the tables it references, so drop in + // reverse to remove referencing tables before the tables they point to + for (const tableName of tables.toReversed()) { + logging.info(`Dropping in-development table: ${tableName}`); + await commands.deleteTable(tableName, knex); + } + + for (const tableName of tables) { + logging.info(`Creating in-development table: ${tableName}`); + await commands.createTable(tableName, knex); + } +} diff --git a/ghost/core/core/server/data/schema/index.js b/ghost/core/core/server/data/schema/index.js index aebd061f0e3..d644534c270 100644 --- a/ghost/core/core/server/data/schema/index.js +++ b/ghost/core/core/server/data/schema/index.js @@ -1,5 +1,6 @@ module.exports.tables = require('./schema'); module.exports.views = require('./views'); module.exports.commands = require('./commands'); +module.exports.inDevelopment = require('./in-development'); module.exports.defaultSettings = require('./default-settings'); module.exports.validate = require('./validator').validateSchema; diff --git a/ghost/core/core/shared/config/defaults.json b/ghost/core/core/shared/config/defaults.json index 01440be1f01..d5c34d3d09e 100644 --- a/ghost/core/core/shared/config/defaults.json +++ b/ghost/core/core/shared/config/defaults.json @@ -373,5 +373,6 @@ }, "disableJSBackups": false, "disableMigrationBackups": false, + "createInDevelopmentTables": false, "usingLoopbackReverseProxy": false } diff --git a/ghost/core/core/shared/config/env/config.development.json b/ghost/core/core/shared/config/env/config.development.json index d1ce3c79799..9c2895cc865 100644 --- a/ghost/core/core/shared/config/env/config.development.json +++ b/ghost/core/core/shared/config/env/config.development.json @@ -1,5 +1,6 @@ { "url": "http://localhost:2368", + "createInDevelopmentTables": true, "mail": { "from": "test@example.com", "transport": "SMTP", diff --git a/ghost/core/core/shared/config/env/config.testing-mysql.json b/ghost/core/core/shared/config/env/config.testing-mysql.json index c9e783c92d4..651542404fe 100644 --- a/ghost/core/core/shared/config/env/config.testing-mysql.json +++ b/ghost/core/core/shared/config/env/config.testing-mysql.json @@ -1,5 +1,6 @@ { "url": "http://127.0.0.1:2369", + "createInDevelopmentTables": true, "server": { "port": 2369 }, diff --git a/ghost/core/core/shared/config/env/config.testing.json b/ghost/core/core/shared/config/env/config.testing.json index 4b4208930ba..4830224a0a4 100644 --- a/ghost/core/core/shared/config/env/config.testing.json +++ b/ghost/core/core/shared/config/env/config.testing.json @@ -1,5 +1,6 @@ { "url": "http://127.0.0.1:2369", + "createInDevelopmentTables": true, "database": { "client": "better-sqlite3", "connection": { diff --git a/ghost/core/package.json b/ghost/core/package.json index fa3b385f2c8..8817446591d 100644 --- a/ghost/core/package.json +++ b/ghost/core/package.json @@ -51,6 +51,7 @@ "generate-golden-email": "node bin/generate-golden-email.js", "query-parameter-policy:export": "node --import=tsx scripts/export-query-parameter-policy.ts", "migrate:create": "node bin/create-migration.js", + "migrate:rebuild-in-development-tables": "node --import=tsx bin/rebuild-in-development-tables.ts", "build:assets:css": "postcss core/frontend/public/ghost.css --no-map --use cssnano -o core/frontend/public/ghost.min.css", "build:tsc": "tsc", "pretest": "pnpm build:assets", diff --git a/ghost/core/test/integration/migrations/in-development-tables.test.ts b/ghost/core/test/integration/migrations/in-development-tables.test.ts new file mode 100644 index 00000000000..0e7538c6624 --- /dev/null +++ b/ghost/core/test/integration/migrations/in-development-tables.test.ts @@ -0,0 +1,174 @@ +import assert from 'node:assert/strict'; +import sinon from 'sinon'; + +// require, not import: these must be the same CommonJS instances that +// in-development.ts loads, so the tables added to the schema, the config set +// here and the spies on commands are the ones it sees +const testUtils = require('../../utils'); +const configUtils = require('../../utils/config-utils'); +const db = require('../../../core/server/data/db'); +const commands = require('../../../core/server/data/schema/commands'); +const schema = require('../../../core/server/data/schema/schema'); +const { addTable } = require('../../../core/server/data/migrations/utils'); +const inDevelopment: typeof import('../../../core/server/data/schema/in-development') = require('../../../core/server/data/schema/in-development'); + +const PARENT = 'in_dev_test_parents'; +const CHILD = 'in_dev_test_children'; + +const TEST_TABLES = { + [PARENT]: { + id: { type: 'string', maxlength: 24, nullable: false, primary: true }, + }, + [CHILD]: { + id: { type: 'string', maxlength: 24, nullable: false, primary: true }, + parent_id: { type: 'string', maxlength: 24, nullable: false, references: `${PARENT}.id` }, + }, +}; + +async function hasTable(tableName: string): Promise { + return db.knex.schema.hasTable(tableName); +} + +async function dropTestTables() { + await db.knex.schema.dropTableIfExists(CHILD); + await db.knex.schema.dropTableIfExists(PARENT); +} + +describe('In-development tables', function () { + let originalTables: string[]; + + beforeAll(async function () { + await testUtils.startGhost(); + }); + + beforeEach(async function () { + Object.assign(schema, TEST_TABLES); + originalTables = [...inDevelopment.IN_DEVELOPMENT_TABLES]; + inDevelopment.IN_DEVELOPMENT_TABLES.splice(0, Infinity, PARENT, CHILD); + await dropTestTables(); + }); + + afterEach(async function () { + sinon.restore(); + await dropTestTables(); + inDevelopment.IN_DEVELOPMENT_TABLES.splice(0, Infinity, ...originalTables); + delete schema[PARENT]; + delete schema[CHILD]; + await configUtils.restore(); + }); + + describe('createMissingInDevelopmentTables', function () { + it('creates missing tables and leaves existing ones alone', async function () { + await commands.createTable(PARENT, db.knex); + await db.knex(PARENT).insert({ id: 'parent' }); + + await inDevelopment.createMissingInDevelopmentTables(db.knex); + + assert.equal(await hasTable(CHILD), true); + assert.deepEqual(await db.knex(PARENT).pluck('id'), ['parent']); + }); + + it('creates nothing when disabled', async function () { + configUtils.set('createInDevelopmentTables', false); + + await inDevelopment.createMissingInDevelopmentTables(db.knex); + + assert.equal(await hasTable(PARENT), false); + assert.equal(await hasTable(CHILD), false); + }); + }); + + describe('rebuildInDevelopmentTables', function () { + it('drops referencing tables first, then recreates them empty', async function () { + await commands.createTable(PARENT, db.knex); + await commands.createTable(CHILD, db.knex); + await db.knex(PARENT).insert({ id: 'parent' }); + await db.knex(CHILD).insert({ id: 'child', parent_id: 'parent' }); + + const deleteTable = sinon.spy(commands, 'deleteTable'); + const createTable = sinon.spy(commands, 'createTable'); + + await inDevelopment.rebuildInDevelopmentTables(db.knex); + + assert.deepEqual( + deleteTable.getCalls().map((call) => call.args[0]), + [CHILD, PARENT], + ); + assert.deepEqual( + createTable.getCalls().map((call) => call.args[0]), + [PARENT, CHILD], + ); + assert.equal((await db.knex(PARENT).pluck('id')).length, 0); + assert.equal((await db.knex(CHILD).pluck('id')).length, 0); + }); + + it('leaves the tables alone when disabled', async function () { + await commands.createTable(PARENT, db.knex); + await db.knex(PARENT).insert({ id: 'parent' }); + configUtils.set('createInDevelopmentTables', false); + + await inDevelopment.rebuildInDevelopmentTables(db.knex); + + assert.deepEqual(await db.knex(PARENT).pluck('id'), ['parent']); + }); + }); + + describe('addTable with replaceDevelopmentCopy', function () { + const finalParentSpec = { + ...TEST_TABLES[PARENT], + name: { type: 'string', maxlength: 191, nullable: true }, + }; + + async function columns(tableName: string): Promise { + return Object.keys(await db.knex(tableName).columnInfo()).sort(); + } + + it('creates the table when there is no copy', async function () { + await addTable(PARENT, finalParentSpec, { replaceDevelopmentCopy: true }).up({ + connection: db.knex, + }); + + assert.deepEqual(await columns(PARENT), ['id', 'name']); + }); + + it('replaces an existing copy, dropping the tables that reference it', async function () { + await commands.createTable(PARENT, db.knex); + await commands.createTable(CHILD, db.knex); + await db.knex(PARENT).insert({ id: 'parent' }); + await db.knex(CHILD).insert({ id: 'child', parent_id: 'parent' }); + + await addTable(PARENT, finalParentSpec, { replaceDevelopmentCopy: true }).up({ + connection: db.knex, + }); + + assert.deepEqual(await columns(PARENT), ['id', 'name']); + assert.equal((await db.knex(PARENT).pluck('id')).length, 0); + assert.equal(await hasTable(CHILD), false); + + await inDevelopment.createMissingInDevelopmentTables(db.knex); + assert.equal(await hasTable(CHILD), true); + }); + + it('rolls back while other tables reference it', async function () { + const migration = addTable(PARENT, finalParentSpec, { replaceDevelopmentCopy: true }); + await migration.up({ connection: db.knex }); + await commands.createTable(CHILD, db.knex); + + await migration.down({ connection: db.knex }); + + assert.equal(await hasTable(PARENT), false); + assert.equal(await hasTable(CHILD), false); + }); + + it('leaves an existing table alone outside development and testing', async function () { + await commands.createTable(PARENT, db.knex); + configUtils.set('env', 'production'); + + await addTable(PARENT, finalParentSpec, { replaceDevelopmentCopy: true }).up({ + connection: db.knex, + }); + + assert.deepEqual(await columns(PARENT), ['id']); + }); + }); +}); diff --git a/ghost/core/test/unit/server/data/schema/in-development.test.ts b/ghost/core/test/unit/server/data/schema/in-development.test.ts new file mode 100644 index 00000000000..1ac35ea236f --- /dev/null +++ b/ghost/core/test/unit/server/data/schema/in-development.test.ts @@ -0,0 +1,98 @@ +import assert from 'node:assert/strict'; +// @ts-expect-error This module lacks type definitions. +import schema from '../../../../../core/server/data/schema/schema'; + +// require, not import: config-utils and in-development must resolve to the same +// CommonJS config instance, so values set here are the ones the module reads +const configUtils = require('../../../../utils/config-utils'); +const inDevelopment: typeof import('../../../../../core/server/data/schema/in-development') = require('../../../../../core/server/data/schema/in-development'); + +type ColumnSpec = { references?: string }; + +describe('In-development schema tables', function () { + afterEach(async function () { + await configUtils.restore(); + }); + + it('only lists tables defined in schema.js', function () { + for (const tableName of inDevelopment.IN_DEVELOPMENT_TABLES) { + assert( + Object.hasOwn(schema, tableName), + `In-development table ${tableName} is not defined in schema.js`, + ); + } + }); + + it('are never referenced by finalised tables', function () { + for (const [tableName, table] of Object.entries>(schema)) { + if (inDevelopment.isInDevelopmentTable(tableName)) { + continue; + } + + for (const [columnName, column] of Object.entries(table)) { + if (!column.references) { + continue; + } + + const referencedTable = column.references.split('.')[0]; + assert( + !inDevelopment.isInDevelopmentTable(referencedTable), + `${tableName}.${columnName} references in-development table ${referencedTable}`, + ); + } + } + }); + + describe('shouldCreateInDevelopmentTables', function () { + it('is enabled in development and testing when configured', function () { + configUtils.set('createInDevelopmentTables', true); + + for (const env of ['development', 'testing', 'testing-mysql']) { + configUtils.set('env', env); + assert.equal(inDevelopment.shouldCreateInDevelopmentTables(), true, env); + } + }); + + it('is disabled when not configured', function () { + configUtils.set('env', 'development'); + configUtils.set('createInDevelopmentTables', false); + + assert.equal(inDevelopment.shouldCreateInDevelopmentTables(), false); + }); + + it('ignores the config outside development and testing', function () { + configUtils.set('env', 'production'); + configUtils.set('createInDevelopmentTables', true); + + assert.equal(inDevelopment.shouldCreateInDevelopmentTables(), false); + }); + }); + + describe('getTablesToCreate', function () { + let originalTables: string[]; + + beforeEach(function () { + originalTables = [...inDevelopment.IN_DEVELOPMENT_TABLES]; + inDevelopment.IN_DEVELOPMENT_TABLES.splice(0, Infinity, 'posts_meta'); + configUtils.set('env', 'development'); + }); + + afterEach(function () { + inDevelopment.IN_DEVELOPMENT_TABLES.splice(0, Infinity, ...originalTables); + }); + + it('includes in-development tables when enabled', function () { + configUtils.set('createInDevelopmentTables', true); + + assert.deepEqual(inDevelopment.getTablesToCreate(), Object.keys(schema)); + }); + + it('excludes in-development tables when disabled', function () { + configUtils.set('createInDevelopmentTables', false); + + const tables = inDevelopment.getTablesToCreate(); + assert(!tables.includes('posts_meta')); + assert.equal(tables.length, Object.keys(schema).length - 1); + }); + }); +}); diff --git a/ghost/core/test/unit/server/data/schema/integrity.test.js b/ghost/core/test/unit/server/data/schema/integrity.test.js index 3809127c273..93936f05e33 100644 --- a/ghost/core/test/unit/server/data/schema/integrity.test.js +++ b/ghost/core/test/unit/server/data/schema/integrity.test.js @@ -5,6 +5,7 @@ const fs = require('fs-extra'); const path = require('path'); const { config } = require('../../../../utils/config-utils'); const schema = require('../../../../../core/server/data/schema/schema'); +const inDevelopment = require('../../../../../core/server/data/schema/in-development'); const fixtures = require('../../../../../core/server/data/schema/fixtures/fixtures.json'); const defaultSettings = require('../../../../../core/server/data/schema/default-settings/default-settings.json'); @@ -55,7 +56,7 @@ describe('DB version integrity', function () { 'yamlSource', ); - const tablesNoValidation = _.cloneDeep(schema); + const tablesNoValidation = _.cloneDeep(_.omit(schema, inDevelopment.IN_DEVELOPMENT_TABLES)); _.each(tablesNoValidation, function (table) { return _.each(table, function (column, name) { From bf68b836e1b5e371551b08adece4b8da0afd01b3 Mon Sep 17 00:00:00 2001 From: Steve Larson <9larsons@gmail.com> Date: Mon, 28 Sep 2026 19:07:24 +0200 Subject: [PATCH 06/21] Fixed publish focus restoration during settings refresh (#31036) no ref Attempt to tighten up a flaky test. Closing Publish during its settings refresh left focus on the document body because the refresh disabled the Publish button ([example failure](https://github.com/TryGhost/Ghost/actions/runs/36449029590/job/109019899264)). Keeping validated cached settings usable during the refresh allows focus to return. --- .../editor/editor-header.acceptance.test.tsx | 60 ++++++++++++++-- .../src/editor/publish/publish.screen.ts | 2 + .../use-publish-inputs.component.test.tsx | 71 +++++++++++++++++++ .../src/editor/publish/use-publish-inputs.ts | 3 +- 4 files changed, 128 insertions(+), 8 deletions(-) diff --git a/apps/admin/src/editor/editor-header.acceptance.test.tsx b/apps/admin/src/editor/editor-header.acceptance.test.tsx index 2532cdc3adb..1e4f57ab708 100644 --- a/apps/admin/src/editor/editor-header.acceptance.test.tsx +++ b/apps/admin/src/editor/editor-header.acceptance.test.tsx @@ -508,6 +508,7 @@ describe('Editor header actions', () => { await expect(previewScreen.modal()).toHaveCount(0); await expect.element(publishScreen.options()).toBeVisible(); + await expect.element(publishScreen.previewButton()).toHaveFocus(); }); it('publishes from a preview opened by the header Preview button', async () => { @@ -614,25 +615,67 @@ describe('Editor header actions', () => { await editorScreen.previewButton().click(); await expect.element(previewScreen.modal()).toBeVisible(); + await expect + .poll(() => previewScreen.modal().element().contains(document.activeElement)) + .toBe(true); + await userEvent.keyboard('{Escape}'); await expect(previewScreen.modal()).toHaveCount(0); await expect.element(editorScreen.previewButton()).toHaveFocus(); }); - it('returns focus to the Publish button when the publish flow closes', async () => { + it.each(['Escape', 'Close button'] as const)( + 'returns focus to Publish after closing with %s and reopening', + async (closeWith) => { + publishChrome(); + fakeSavablePost(); + await renderAdminApp(`/editor/post/${POST_ID}`, FLAG_ON); + + await expect.element(editorScreen.publishButton()).toBeEnabled(); + for (let opening = 0; opening < 2; opening += 1) { + await editorScreen.publishButton().click(); + await expect.element(publishScreen.options()).toBeVisible(); + await expect + .poll(() => publishScreen.root().element().contains(document.activeElement)) + .toBe(true); + + if (closeWith === 'Escape') { + await userEvent.keyboard('{Escape}'); + } else { + await publishScreen.closeButton().click(); + } + + await expect(publishScreen.root()).toHaveCount(0); + await expect.element(editorScreen.publishButton()).toHaveFocus(); + } + }, + ); + + it('returns focus to Publish when closed during the publish settings refresh', async () => { publishChrome(); fakeSavablePost(); await renderAdminApp(`/editor/post/${POST_ID}`, FLAG_ON); - await expect.element(editorScreen.publishButton()).toBeEnabled(); - await editorScreen.publishButton().click(); - await expect.element(publishScreen.options()).toBeVisible(); - await userEvent.keyboard('{Escape}'); + const refreshedSettings = deferred(); + const settingsApi = fakeAdminEndpoint('GET', /^\/settings\//, async () => { + await refreshedSettings.promise; + return settingsResponse({ labs: FLAG_ON.labs }); + }); + + try { + await editorScreen.publishButton().click(); + await expect.element(publishScreen.options()).toBeVisible(); + await expect.poll(() => settingsApi.requests.length).toBeGreaterThan(0); + await expect.element(editorScreen.publishButton()).toBeEnabled(); - await expect(publishScreen.root()).toHaveCount(0); - await expect.element(editorScreen.publishButton()).toHaveFocus(); + await userEvent.keyboard('{Escape}'); + await expect(publishScreen.root()).toHaveCount(0); + await expect.element(editorScreen.publishButton()).toHaveFocus(); + } finally { + refreshedSettings.resolve(); + } }); it('returns focus to the Unpublish button when the update flow closes', async () => { @@ -642,6 +685,9 @@ describe('Editor header actions', () => { await editorScreen.unpublishButton().click(); await expect.element(publishScreen.updateFlow()).toBeVisible(); + await expect + .poll(() => publishScreen.updateFlow().element().contains(document.activeElement)) + .toBe(true); await userEvent.keyboard('{Escape}'); diff --git a/apps/admin/src/editor/publish/publish.screen.ts b/apps/admin/src/editor/publish/publish.screen.ts index e5a2d6508c8..c1458ff5b4c 100644 --- a/apps/admin/src/editor/publish/publish.screen.ts +++ b/apps/admin/src/editor/publish/publish.screen.ts @@ -37,6 +37,8 @@ const SETTINGS = { /** Publish and update flow locators and gestures for acceptance specs; no assertions. */ export const publishScreen = { root: () => page.getByTestId(publishFlowModal), + closeButton: () => + page.getByTestId(publishFlowModal).getByRole('button', { name: 'Close', exact: true }), options: () => page.getByTestId(publishFlowOptions), confirm: () => page.getByTestId(publishFlowConfirm), complete: () => page.getByTestId(publishFlowComplete), diff --git a/apps/admin/src/editor/publish/use-publish-inputs.component.test.tsx b/apps/admin/src/editor/publish/use-publish-inputs.component.test.tsx index acb0f3119e8..ee5107a1978 100644 --- a/apps/admin/src/editor/publish/use-publish-inputs.component.test.tsx +++ b/apps/admin/src/editor/publish/use-publish-inputs.component.test.tsx @@ -4,6 +4,7 @@ import { renderHook } from 'vitest-browser-react'; import { InAppProviders, fakeAdminEndpoint, newsletter } from '@test-utils/acceptance'; import { usePublishInputs } from '@/editor/publish/use-publish-inputs'; +import { useEditorSettings } from '@/editor/use-editor-settings'; const pagination = (pageNumber = 1, pages = 1) => ({ page: pageNumber, @@ -62,6 +63,76 @@ function fakeMemberCount(total: number, status = 200) { } describe('usePublishInputs', () => { + it('keeps valid inputs ready while settings refresh in the background', async () => { + fakeBoundaryInputs(); + fakeNewsletters(); + fakeMemberCount(20); + const hook = await renderHook( + () => ({ + ...usePublishInputs(), + refetchSettings: useEditorSettings().refetch, + }), + { wrapper: InAppProviders }, + ); + await expect.poll(() => hook.result.current.isReady).toBe(true); + + let releaseSettings: () => void = () => {}; + const settingsHeld = new Promise((resolve) => { + releaseSettings = resolve; + }); + const refreshedSettings = fakeAdminEndpoint('GET', /^\/settings\/\?/, async () => { + await settingsHeld; + return { + settings: [ + { key: 'members_signup_access', value: 'none' }, + { key: 'editor_default_email_recipients', value: 'visibility' }, + { key: 'timezone', value: 'Etc/UTC' }, + ], + }; + }); + + try { + await hook.act(() => { + void hook.result.current.refetchSettings(); + }); + await expect.poll(() => refreshedSettings.requests.length).toBe(1); + + expect(hook.result.current.isReady).toBe(true); + expect(hook.result.current.site.membersEnabled).toBe(true); + expect(hook.result.current.error).toBeNull(); + } finally { + releaseSettings(); + } + + await expect.poll(() => hook.result.current.site.membersEnabled).toBe(false); + expect(hook.result.current.isReady).toBe(true); + }); + + it.each(['failed', 'invalid'] as const)( + 'blocks previously ready inputs after a %s settings refresh', + async (response) => { + fakeBoundaryInputs(); + fakeNewsletters(); + fakeMemberCount(20); + const hook = await renderHook(() => usePublishInputs(), { wrapper: InAppProviders }); + await expect.poll(() => hook.result.current.isReady).toBe(true); + + const refreshedSettings = fakeAdminEndpoint( + 'GET', + /^\/settings\/\?/, + response === 'failed' + ? { errors: [{ message: 'Settings are offline' }] } + : { settings: [{ key: 'editor_default_email_recipients', value: 'invalid' }] }, + { status: response === 'failed' ? 500 : 200 }, + ); + await hook.act(() => hook.result.current.retry()); + + await expect.poll(() => refreshedSettings.requests.length).toBe(1); + await expect.poll(() => hook.result.current.error !== null).toBe(true); + expect(hook.result.current.isReady).toBe(false); + }, + ); + it('blocks on a member-count error and becomes ready after retry', async () => { const inputs = fakeBoundaryInputs(); fakeNewsletters(); diff --git a/apps/admin/src/editor/publish/use-publish-inputs.ts b/apps/admin/src/editor/publish/use-publish-inputs.ts index 75956c2d6c9..182ccda4c46 100644 --- a/apps/admin/src/editor/publish/use-publish-inputs.ts +++ b/apps/admin/src/editor/publish/use-publish-inputs.ts @@ -201,8 +201,9 @@ export function usePublishInputs(): PublishInputs { [settingsData, configData, newslettersData, currentUser, memberCount], ); const isLoading = + // Opening the flow refreshes settings for its limit checks. Keep validated + // cached settings usable so its Publish opener can receive focus on close. settingsQuery.isLoading || - settingsQuery.isFetching || configQuery.isLoading || configQuery.isFetching || newslettersQuery.isLoading || From 086507f53a15fdb39438fb16681b3b418e5f4c03 Mon Sep 17 00:00:00 2001 From: Steve Larson <9larsons@gmail.com> Date: Mon, 28 Sep 2026 19:37:50 +0200 Subject: [PATCH 07/21] Moved the View site and migration screens from Ember to React (#31003) no ref Moves the View site (`/site`) and migration (`/migrate/*`) screens to React and deletes the Ember versions. Both screens just host an iframe, so they ship without a flag. This contains some small stylistic updates (consistency fixes) to the migrate iframe. --- .../layout/admin7-design.acceptance.test.tsx | 2 +- .../src/layout/sidebar.acceptance.test.tsx | 2 + apps/admin/src/migrate/api.ts | 8 + .../src/migrate/migrate.acceptance.test.tsx | 226 ++++++++++++++++++ apps/admin/src/migrate/migrate.screen.ts | 7 + apps/admin/src/migrate/migrate.tsx | 131 ++++++++++ .../src/route-access.acceptance.test.tsx | 20 ++ apps/admin/src/routes.tsx | 14 +- .../components/exit-settings-button.tsx | 14 +- .../src/settings/layout/main-content.tsx | 7 +- .../src/shared/fullscreen-close-button.tsx | 20 ++ apps/admin/src/view-site/api.ts | 8 + .../view-site/view-site.acceptance.test.tsx | 54 +++++ apps/admin/src/view-site/view-site.screen.ts | 8 + apps/admin/src/view-site/view-site.tsx | 42 ++++ apps/admin/test-utils/acceptance/README.md | 2 + apps/admin/test-utils/acceptance/frames.ts | 27 +++ apps/admin/test-utils/acceptance/index.ts | 1 + apps/admin/test-utils/acceptance/setup.ts | 8 +- apps/admin/vitest.acceptance.config.ts | 45 ++++ .../app/components/gh-migrate-iframe.hbs | 1 - .../app/components/gh-migrate-iframe.js | 88 ------- .../app/components/gh-migrate-modal.hbs | 5 - .../app/components/gh-migrate-modal.js | 10 - .../app/components/gh-site-iframe.hbs | 12 - .../app/components/gh-site-iframe.js | 102 -------- apps/ember-admin/app/controllers/migrate.js | 17 -- apps/ember-admin/app/controllers/site.js | 9 - apps/ember-admin/app/router.js | 6 - apps/ember-admin/app/routes/migrate.js | 16 -- apps/ember-admin/app/routes/site.js | 13 - apps/ember-admin/app/services/migrate.js | 135 ----------- apps/ember-admin/app/styles/app.css | 1 - .../app/styles/components/browser-preview.css | 23 -- .../app/styles/layouts/migrate.css | 57 ----- .../app/styles/patterns/global.css | 12 - apps/ember-admin/app/templates/migrate.hbs | 9 - apps/ember-admin/app/templates/site.hbs | 1 - .../components/gh-site-iframe-test.js | 23 -- .../tests/unit/services/migrate-test.js | 74 ------ e2e/helpers/pages/admin/index.ts | 1 + e2e/helpers/pages/admin/migrate-page.ts | 17 ++ e2e/helpers/pages/admin/site-page.ts | 3 +- .../migration-app-handoff.test.ts | 62 +++++ .../admin/iframe-routes/view-site.test.ts | 15 ++ .../test-data/src/selectors/migrate.ts | 8 + .../test-data/src/selectors/view-site.ts | 8 + 47 files changed, 739 insertions(+), 635 deletions(-) create mode 100644 apps/admin/src/migrate/api.ts create mode 100644 apps/admin/src/migrate/migrate.acceptance.test.tsx create mode 100644 apps/admin/src/migrate/migrate.screen.ts create mode 100644 apps/admin/src/migrate/migrate.tsx create mode 100644 apps/admin/src/shared/fullscreen-close-button.tsx create mode 100644 apps/admin/src/view-site/api.ts create mode 100644 apps/admin/src/view-site/view-site.acceptance.test.tsx create mode 100644 apps/admin/src/view-site/view-site.screen.ts create mode 100644 apps/admin/src/view-site/view-site.tsx create mode 100644 apps/admin/test-utils/acceptance/frames.ts delete mode 100644 apps/ember-admin/app/components/gh-migrate-iframe.hbs delete mode 100644 apps/ember-admin/app/components/gh-migrate-iframe.js delete mode 100644 apps/ember-admin/app/components/gh-migrate-modal.hbs delete mode 100644 apps/ember-admin/app/components/gh-migrate-modal.js delete mode 100644 apps/ember-admin/app/components/gh-site-iframe.hbs delete mode 100644 apps/ember-admin/app/components/gh-site-iframe.js delete mode 100644 apps/ember-admin/app/controllers/migrate.js delete mode 100644 apps/ember-admin/app/controllers/site.js delete mode 100644 apps/ember-admin/app/routes/migrate.js delete mode 100644 apps/ember-admin/app/routes/site.js delete mode 100644 apps/ember-admin/app/services/migrate.js delete mode 100644 apps/ember-admin/app/styles/layouts/migrate.css delete mode 100644 apps/ember-admin/app/templates/migrate.hbs delete mode 100644 apps/ember-admin/app/templates/site.hbs delete mode 100644 apps/ember-admin/tests/integration/components/gh-site-iframe-test.js delete mode 100644 apps/ember-admin/tests/unit/services/migrate-test.js create mode 100644 e2e/helpers/pages/admin/migrate-page.ts create mode 100644 e2e/tests/admin/iframe-routes/migration-app-handoff.test.ts create mode 100644 e2e/tests/admin/iframe-routes/view-site.test.ts create mode 100644 packages/testing/test-data/src/selectors/migrate.ts create mode 100644 packages/testing/test-data/src/selectors/view-site.ts diff --git a/apps/admin/src/layout/admin7-design.acceptance.test.tsx b/apps/admin/src/layout/admin7-design.acceptance.test.tsx index 8b8066334cb..f61ef27fe4f 100644 --- a/apps/admin/src/layout/admin7-design.acceptance.test.tsx +++ b/apps/admin/src/layout/admin7-design.acceptance.test.tsx @@ -16,7 +16,7 @@ it.each<{ { name: 'flag absent', route: '/members', labs: {}, enabled: false }, { name: 'flag disabled', route: '/members', labs: { admin7Pill: false }, enabled: false }, { name: 'flag enabled', route: '/members', labs: { admin7Pill: true }, enabled: true }, - { name: 'Ember route excluded', route: '/site', labs: { admin7Pill: true }, enabled: false }, + { name: 'Ember route excluded', route: '/restore', labs: { admin7Pill: true }, enabled: false }, { name: 'Ember editor excluded', route: '/editor/post/new', diff --git a/apps/admin/src/layout/sidebar.acceptance.test.tsx b/apps/admin/src/layout/sidebar.acceptance.test.tsx index bfb111fc261..92fe7500a5a 100644 --- a/apps/admin/src/layout/sidebar.acceptance.test.tsx +++ b/apps/admin/src/layout/sidebar.acceptance.test.tsx @@ -112,6 +112,8 @@ describe('Sidebar navigation', () => { it('keeps the boot loader visible until React commits its mount marker', async () => { await renderAdminApp('/site'); + // `/site` is a lazy route, so the shell commits after its chunk loads. + await expect.element(sidebarScreen.shellNav()).toBeVisible(); const marker = document.querySelector('[data-react-admin-mounted]')!; const emberApp = document.getElementById('ember-app')!; diff --git a/apps/admin/src/migrate/api.ts b/apps/admin/src/migrate/api.ts new file mode 100644 index 00000000000..3ccf91006c9 --- /dev/null +++ b/apps/admin/src/migrate/api.ts @@ -0,0 +1,8 @@ +/** + * Public surface of the migrate domain, consumed by the admin shell + * (apps/admin/src/routes.tsx). Everything else in this domain is internal. + */ + +// Lazy entry, not a component re-export: the shell mounts it behind `lazy()`, +// so a static re-export would pull the chunk into the shell bundle. +export const lazyMigrateScreen = () => import('./migrate'); diff --git a/apps/admin/src/migrate/migrate.acceptance.test.tsx b/apps/admin/src/migrate/migrate.acceptance.test.tsx new file mode 100644 index 00000000000..44df16c5dd5 --- /dev/null +++ b/apps/admin/src/migrate/migrate.acceptance.test.tsx @@ -0,0 +1,226 @@ +import { beforeEach, describe, expect, it, onTestFinished } from 'vitest'; +import { page } from 'vitest/browser'; +import { + allowUnhandledRequests, + configResponse, + currentRoute, + fakeFrameOrigin, + fakeIntegrations, + fakeUsers, + renderAdminApp, + settingsResponse, + staffRole, + staffUser, +} from '@test-utils/acceptance'; +import type { Integration } from '@tryghost/admin-x-framework/api/integrations'; +import { sidebarScreen } from '@/layout/sidebar.screen'; +import { migrateScreen } from './migrate.screen'; + +const MIGRATE_ORIGIN = 'https://migrate.ghost.org'; + +// Stands in for the migration app: reports each load with its own id, echoes +// pings with that id, relays every other message back to Admin's window, then +// runs `script`. +function migrateStandIn(script = '') { + return ``; +} + +interface StandInMessage { + loaded?: string; + loadId?: string; + pong?: string; + received?: { request: string; response: Record }; +} + +function standInMessages(): StandInMessage[] { + const messages: StandInMessage[] = []; + const listener = (event: MessageEvent) => { + const data = event.data; + if ( + event.origin === MIGRATE_ORIGIN && + data && + ('loaded' in data || 'pong' in data || 'received' in data) + ) { + messages.push(data); + } + }; + window.addEventListener('message', listener); + onTestFinished(() => window.removeEventListener('message', listener)); + return messages; +} + +function selfServeMigration(secret: string): Integration { + const created = '2024-01-01T00:00:00.000Z'; + return { + id: 'ssm-id', + type: 'core', + slug: 'self-serve-migration', + name: 'Self-Serve Migration Integration', + icon_image: null, + description: null, + created_at: created, + updated_at: created, + api_keys: [ + { + id: 'ssm-key-id', + type: 'admin', + secret, + role_id: 'role-id', + integration_id: 'ssm-id', + user_id: null, + last_seen_at: null, + last_seen_version: null, + created_at: created, + updated_at: created, + }, + ], + }; +} + +const siteOwner = () => + staffUser({ email: 'owner@example.com', roles: [staffRole({ name: 'Owner' })] }); + +const externalNavigation = (): unknown => + JSON.parse(document.body.dataset.externalNavigate ?? 'null'); + +describe('Migrate', () => { + // The recorded handoff lives on the host page, which outlives a single test. + beforeEach(() => { + delete document.body.dataset.externalNavigate; + }); + + it('opens the migration app for the chosen platform', async () => { + await fakeFrameOrigin(MIGRATE_ORIGIN, migrateStandIn()); + const messages = standInMessages(); + await renderAdminApp('/migrate/substack'); + + await expect.poll(() => messages[0]?.loaded).toBe(`${MIGRATE_ORIGIN}/?platform=substack`); + await expect.element(sidebarScreen.shellNav()).not.toBeInTheDocument(); + }); + + it('sends the migration app its credentials when asked', async () => { + const users = fakeUsers([siteOwner()]); + fakeIntegrations([selfServeMigration('ssm-secret')]); + await fakeFrameOrigin( + MIGRATE_ORIGIN, + migrateStandIn(`parent.postMessage({ request: 'apiUrl' }, '*');`), + ); + const messages = standInMessages(); + await renderAdminApp('/migrate', { + boot: { + browseSettings: { + response: settingsResponse({ + settings: { + stripe_connect_account_id: 'acct_123', + stripe_connect_publishable_key: 'pk_live_123', + stripe_connect_livemode: true, + }, + }), + }, + }, + }); + + await expect + .poll(() => messages.find((message) => message.received)?.received) + .toEqual({ + request: 'initialData', + response: { + apiUrl: `${window.location.origin}/ghost`, + apiKey: 'ssm-secret', + stripe: true, + csvContentImporter: false, + ghostVersion: String(configResponse().config.version).split('.').slice(0, 2).join('.'), + ownerEmail: 'owner@example.com', + }, + }); + await expect(users).toHaveSentFilter("roles.name:'Owner'"); + }); + + it('returns to migration settings with an error when the credentials cannot be loaded', async () => { + // The settings app owns its request graph; this spec asserts the handoff. + allowUnhandledRequests(); + fakeUsers([siteOwner()]); + fakeIntegrations([]); + await fakeFrameOrigin( + MIGRATE_ORIGIN, + migrateStandIn(`parent.postMessage({ request: 'apiUrl' }, '*');`), + ); + await renderAdminApp('/migrate'); + + await expect.poll(currentRoute).toBe('/settings/migration'); + await expect + .element(page.getByText('Error initialising migration. Please try again later.')) + .toBeVisible(); + }); + + it('follows migration routes without reloading the app', async () => { + await fakeFrameOrigin( + MIGRATE_ORIGIN, + migrateStandIn(`parent.postMessage({ route: '/migrate/beehiiv' }, '*');`), + ); + const messages = standInMessages(); + await renderAdminApp('/migrate/substack'); + await expect.poll(currentRoute).toBe('/migrate/beehiiv'); + + const frame = migrateScreen.frame().element() as HTMLIFrameElement; + frame.contentWindow?.postMessage({ ping: true }, MIGRATE_ORIGIN); + + await expect + .poll(() => messages.find((message) => message.pong)?.pong) + .toBe(messages[0]?.loadId); + expect(messages.filter((message) => message.loaded)).toHaveLength(1); + }); + + it('hands routes Ember owns to Ember', async () => { + await fakeFrameOrigin( + MIGRATE_ORIGIN, + migrateStandIn(`parent.postMessage({ route: '/pro' }, '*');`), + ); + await renderAdminApp('/migrate'); + + await expect.poll(externalNavigation).toMatchObject({ route: '/pro', isExternal: true }); + }); + + it('ignores messages from other origins', async () => { + await fakeFrameOrigin(MIGRATE_ORIGIN, migrateStandIn()); + const messages = standInMessages(); + await renderAdminApp('/migrate'); + await expect.poll(() => messages.length).toBeGreaterThan(0); + + // Handled in order, so honouring the second would supersede the first. + window.dispatchEvent( + new MessageEvent('message', { origin: MIGRATE_ORIGIN, data: { route: '/migrate/beehiiv' } }), + ); + window.dispatchEvent( + new MessageEvent('message', { + origin: 'https://example.com', + data: { route: '/migrate/wordpress' }, + }), + ); + + await expect.poll(currentRoute).toBe('/migrate/beehiiv'); + }); + + it('closes to migration settings', async () => { + // The settings app owns its request graph; this spec asserts the handoff. + allowUnhandledRequests(); + await fakeFrameOrigin(MIGRATE_ORIGIN, migrateStandIn()); + await renderAdminApp('/migrate/substack'); + + await migrateScreen.closeButton().click(); + + await expect.poll(currentRoute).toBe('/settings/migration'); + await expect.element(migrateScreen.frame()).not.toBeInTheDocument(); + }); +}); diff --git a/apps/admin/src/migrate/migrate.screen.ts b/apps/admin/src/migrate/migrate.screen.ts new file mode 100644 index 00000000000..f21be190d8b --- /dev/null +++ b/apps/admin/src/migrate/migrate.screen.ts @@ -0,0 +1,7 @@ +import { page } from 'vitest/browser'; +import { closeMigrateButton, migrateFrame } from '@tryghost/test-data/selectors/migrate'; + +export const migrateScreen = { + frame: () => page.getByTitle(migrateFrame, { exact: true }), + closeButton: () => page.getByRole('button', { name: closeMigrateButton, exact: true }), +}; diff --git a/apps/admin/src/migrate/migrate.tsx b/apps/admin/src/migrate/migrate.tsx new file mode 100644 index 00000000000..f858c97be5d --- /dev/null +++ b/apps/admin/src/migrate/migrate.tsx @@ -0,0 +1,131 @@ +import { useEffect, useRef, useState } from 'react'; +import { toast } from 'sonner'; +import { useNavigate, useParams } from '@tryghost/admin-x-framework'; +import { useBrowseConfig } from '@tryghost/admin-x-framework/api/config'; +import type { IntegrationsResponseType } from '@tryghost/admin-x-framework/api/integrations'; +import { getSettingValue, useBrowseSettings } from '@tryghost/admin-x-framework/api/settings'; +import type { UsersResponseType } from '@tryghost/admin-x-framework/api/users'; +import { apiUrl, getGhostPaths } from '@tryghost/admin-x-framework/helpers'; +import { useFeatureFlag, useFetchApi } from '@tryghost/admin-x-framework/hooks'; +import { useEmberOwnedRouteMatcher } from '@/routes'; +import { FullscreenCloseButton } from '@/shared/fullscreen-close-button'; + +const MIGRATE_ORIGIN = 'https://migrate.ghost.org'; + +interface MigrateMessage { + request?: unknown; + route?: unknown; +} + +function migrateFrameUrl(platform: string | undefined): string { + return platform ? `${MIGRATE_ORIGIN}?platform=${platform}` : MIGRATE_ORIGIN; +} + +/** + * The self-serve migration app, embedded fullscreen. The app asks for the + * credentials it imports with (`{request: 'apiUrl'}`) and can send Admin to + * another route (`{route}`). + */ +const Migrate = () => { + const { '*': platform } = useParams(); + const [src] = useState(() => migrateFrameUrl(platform || undefined)); + const frameRef = useRef(null); + const navigate = useNavigate(); + const isEmberOwned = useEmberOwnedRouteMatcher(); + const fetchApi = useFetchApi(); + const { data: configData } = useBrowseConfig(); + const { data: settingsData } = useBrowseSettings(); + const csvContentImporter = useFeatureFlag('csvContentImporter'); + + const settings = settingsData?.settings ?? null; + const stripe = Boolean( + getSettingValue(settings, 'stripe_connect_account_id') && + getSettingValue(settings, 'stripe_connect_publishable_key') && + getSettingValue(settings, 'stripe_connect_livemode'), + ); + const ghostVersion = configData?.config.version.match(/^(\d+\.)?(\d+)/)?.[0]; + + useEffect(() => { + const sendInitialData = async () => { + try { + const [{ integrations }, { users }] = await Promise.all([ + fetchApi(apiUrl('/integrations/', { include: 'api_keys' })), + fetchApi( + apiUrl('/users/', { filter: "roles.name:'Owner'", limit: '1', include: 'roles' }), + ), + ]); + const apiKey = integrations.find( + (integration) => integration.slug === 'self-serve-migration', + )?.api_keys?.[0]?.secret; + const ownerEmail = users[0]?.email; + + if (!apiKey || !ownerEmail) { + throw new Error('The self-serve migration integration or site owner is missing'); + } + + frameRef.current?.contentWindow?.postMessage( + { + request: 'initialData', + response: { + apiUrl: `${window.location.origin}${getGhostPaths().adminRoot}`.replace(/\/$/, ''), + apiKey, + stripe, + csvContentImporter, + ghostVersion, + ownerEmail, + }, + }, + MIGRATE_ORIGIN, + ); + } catch { + // Leaving the screen is only right while it is still open. + if (!frameRef.current) { + return; + } + navigate('/settings/migration'); + toast.error('Error initialising migration. Please try again later.'); + } + }; + + const handleMessage = (event: MessageEvent) => { + if (event.origin !== MIGRATE_ORIGIN) { + return; + } + + if (event.data?.request === 'apiUrl') { + void sendInitialData(); + return; + } + + const route = event.data?.route; + if (typeof route === 'string') { + navigate(route, { crossApp: isEmberOwned(route) }); + } + }; + + window.addEventListener('message', handleMessage); + return () => window.removeEventListener('message', handleMessage); + }, [csvContentImporter, fetchApi, ghostVersion, isEmberOwned, navigate, stripe]); + + return ( +
+ {/* The credentials reply reads settings and config, so the app waits for both. */} + {settingsData && configData && ( + \ No newline at end of file diff --git a/apps/ember-admin/app/components/gh-migrate-iframe.js b/apps/ember-admin/app/components/gh-migrate-iframe.js deleted file mode 100644 index 7d4b29fb7cc..00000000000 --- a/apps/ember-admin/app/components/gh-migrate-iframe.js +++ /dev/null @@ -1,88 +0,0 @@ -import Component from '@glimmer/component'; -import {action} from '@ember/object'; -import {htmlSafe} from '@ember/template'; -import {inject as service} from '@ember/service'; - -export default class GhMigrateIframe extends Component { - @service migrate; - @service router; - @service feature; - @service notifications; - - willDestroy() { - super.willDestroy(...arguments); - window.removeEventListener('message', this.handleIframeMessage); - - // Remove the class that adds a higher z-index that is added on setup - document.getElementById('ember-app').classList.remove('migration-app-open'); - } - - @action - setup() { - this.migrate.getMigrateIframe().src = this.migrate.getIframeURL(); - window.addEventListener('message', this.handleIframeMessage); - - // Add class that adds a higher z-index so the app site over the top of the nav bar. #ember-app is a - // few levels up from this component, document.getElementById is a light-touch way to add the class. - document.getElementById('ember-app').classList.add('migration-app-open'); - } - - @action - async handleIframeMessage(event) { - if (this.isDestroyed || this.isDestroying) { - return; - } - - // Only process messages coming from the migrate iframe - const url = new URL(this.migrate.getIframeURL()); - if (event.origin === url.origin) { - if (event.data?.request === 'apiUrl') { - await this._handleUrlRequest(); - return; - } - - if (event.data?.route) { - this._handleRouteUpdate(event.data); - return; - } - - if (event.data?.siteData) { - this._handleSiteDataUpdate(event.data); - return; - } - } - } - - // The iframe can send route updates to navigate to within Admin, as some routes - // have to be rendered within the iframe and others require to break out of it. - _handleRouteUpdate(data) { - const route = data.route; - this.migrate.isIframeTransition = route?.includes('/migrate'); - this.migrate.toggleMigrateWindow(this.migrate.isIframeTransition); - this.router.transitionTo(route); - } - - async _handleUrlRequest() { - try { - const response = await this.migrate.postMessagePayload(); - - this.migrate.getMigrateIframe().contentWindow.postMessage({ - request: 'initialData', - response - }, new URL(this.migrate.getIframeURL()).origin); - } catch (err) { - // Close the iframe so the user can see the notification - this.migrate.closeMigrateWindow(); - this.notifications.showAlert(htmlSafe(`Error initialising migration. Please try again later.`), {type: 'error', key: 'migrate.iframe-postMessage.error'}); - } - } - - _handleSiteDataUpdate(data) { - this.migrate.siteData = data?.siteData ?? {}; - - if (this.migrate.siteData?.migrationComplete) { - // If we want to show a notification, this is where to do it - // this.notifications.showAlert(htmlSafe(`Migration complete!`), {type: 'success', key: 'migrate.completed'}); // Green persistent banner at the top - } - } -} diff --git a/apps/ember-admin/app/components/gh-migrate-modal.hbs b/apps/ember-admin/app/components/gh-migrate-modal.hbs deleted file mode 100644 index 47d38594abc..00000000000 --- a/apps/ember-admin/app/components/gh-migrate-modal.hbs +++ /dev/null @@ -1,5 +0,0 @@ -
-
- -
-
\ No newline at end of file diff --git a/apps/ember-admin/app/components/gh-migrate-modal.js b/apps/ember-admin/app/components/gh-migrate-modal.js deleted file mode 100644 index 0beac5ec606..00000000000 --- a/apps/ember-admin/app/components/gh-migrate-modal.js +++ /dev/null @@ -1,10 +0,0 @@ -import Component from '@glimmer/component'; -import {inject as service} from '@ember/service'; - -export default class GhMigrateModal extends Component { - @service migrate; - - get visibilityClass() { - return this.migrate.migrateWindowOpen ? 'gh-migrate' : 'gh-migrate closed'; - } -} diff --git a/apps/ember-admin/app/components/gh-site-iframe.hbs b/apps/ember-admin/app/components/gh-site-iframe.hbs deleted file mode 100644 index 182c272c6fe..00000000000 --- a/apps/ember-admin/app/components/gh-site-iframe.hbs +++ /dev/null @@ -1,12 +0,0 @@ - diff --git a/apps/ember-admin/app/components/gh-site-iframe.js b/apps/ember-admin/app/components/gh-site-iframe.js deleted file mode 100644 index 8310c79fbc0..00000000000 --- a/apps/ember-admin/app/components/gh-site-iframe.js +++ /dev/null @@ -1,102 +0,0 @@ -import Component from '@glimmer/component'; -import {action} from '@ember/object'; -import {inject} from 'ghost-admin/decorators/inject'; -import {task, timeout} from 'ember-concurrency'; -import {tracked} from '@glimmer/tracking'; - -export default class GhSiteIframeComponent extends Component { - @inject config; - - @tracked isInvisible = this.args.invisibleUntilLoaded; - - willDestroy() { - super.willDestroy?.(...arguments); - - if (this.messageListener) { - window.removeEventListener('message', this.messageListener); - } - this.args.onDestroyed?.(); - } - - get srcUrl() { - const srcUrl = new URL(this.args.src || `${this.config.blogUrl}/`); - - if (this.args.guid) { - srcUrl.searchParams.set('v', this.args.guid); - } - - srcUrl.searchParams.set('admin', '1'); - srcUrl.searchParams.set('admin_toolbar', '0'); - - return srcUrl.href; - } - - @action - resetSrcAttribute(iframe) { - // reset the src attribute and force reload each time the guid changes - // - allows for a click on the navigation item to reset back to the homepage - // or a portal preview modal to force a reload so it can fetch server-side data - if (this.args.guid !== this._lastGuid) { - if (iframe) { - if (this.args.invisibleUntilLoaded) { - this.isInvisible = true; - } - - try { - if (iframe.contentWindow.location.href !== this.srcUrl) { - iframe.contentWindow.location = this.srcUrl; - } else { - iframe.contentWindow.location.reload(); - } - } catch (e) { - if (e.name === 'SecurityError') { - iframe.src = this.srcUrl; - } - } - } - } - this._lastGuid = this.args.guid; - } - - @action - onLoad(event) { - this.iframe = event.target; - - if (this.args.invisibleUntilLoaded && typeof this.args.invisibleUntilLoaded === 'boolean') { - this.makeVisible.perform(); - } else { - this.args.onLoad?.(this.iframe); - } - } - - @action - attachMessageListener() { - if (typeof this.args.invisibleUntilLoaded === 'string') { - this.messageListener = (event) => { - if (this.isDestroying || this.isDestroyed) { - return; - } - - const srcURL = new URL(this.srcUrl); - const originURL = new URL(event.origin); - - if (originURL.origin === srcURL.origin) { - if (event.data === this.args.invisibleUntilLoaded || event.data.type === this.args.invisibleUntilLoaded) { - this.makeVisible.perform(); - } - } - }; - - window.addEventListener('message', this.messageListener, true); - } - } - - @task - *makeVisible() { - // give any scripts a bit of time to render before making visible - // allows portal to render it's overlay and prevent site background flashes - yield timeout(100); - this.isInvisible = false; - this.args.onLoad?.(this.iframe); - } -} diff --git a/apps/ember-admin/app/controllers/migrate.js b/apps/ember-admin/app/controllers/migrate.js deleted file mode 100644 index 4aa0628652b..00000000000 --- a/apps/ember-admin/app/controllers/migrate.js +++ /dev/null @@ -1,17 +0,0 @@ -import Controller from '@ember/controller'; -import {action} from '@ember/object'; -import {inject as service} from '@ember/service'; - -export default class MigrateController extends Controller { - @service migrate; - @service router; - - get visibilityClass() { - return this.migrate.isIframeTransition ? 'migrate iframe-migrate-container' : ' migrate fullscreen-migrate-container'; - } - - @action - closeMigrate() { - this.router.transitionTo('/settings/migration'); - } -} diff --git a/apps/ember-admin/app/controllers/site.js b/apps/ember-admin/app/controllers/site.js deleted file mode 100644 index ea1b193c608..00000000000 --- a/apps/ember-admin/app/controllers/site.js +++ /dev/null @@ -1,9 +0,0 @@ -import Controller from '@ember/controller'; -import classic from 'ember-classic-decorator'; -import {alias} from '@ember/object/computed'; - -@classic -export default class SiteController extends Controller { - @alias('model') - guid; -} diff --git a/apps/ember-admin/app/router.js b/apps/ember-admin/app/router.js index 3a91492db3c..2bdc2f51081 100644 --- a/apps/ember-admin/app/router.js +++ b/apps/ember-admin/app/router.js @@ -17,8 +17,6 @@ Router.map(function () { this.route('signup', {path: '/signup/:token'}); this.route('reset', {path: '/reset/:token'}); - this.route('site'); - this.route('pro', function () { this.route('pro-sub', {path: '/*sub'}); }); @@ -33,10 +31,6 @@ Router.map(function () { this.route('edit', {path: ':type/:post_id'}); }); - this.route('migrate', function () { - this.route('migrate', {path: '/*platform'}); - }); - this.route('members-activity'); this.route('react-fallback', {path: '/*path'}); diff --git a/apps/ember-admin/app/routes/migrate.js b/apps/ember-admin/app/routes/migrate.js deleted file mode 100644 index d3158731ce8..00000000000 --- a/apps/ember-admin/app/routes/migrate.js +++ /dev/null @@ -1,16 +0,0 @@ -import AuthenticatedRoute from 'ghost-admin/routes/authenticated'; -import {inject as service} from '@ember/service'; - -export default class MigrateRoute extends AuthenticatedRoute { - @service feature; - @service session; - - beforeModel() { - super.beforeModel(...arguments); - - // Only allow Owner & Administrator to access this route - if (!this.session.user.isAdmin) { - return this.transitionTo('index'); - } - } -} diff --git a/apps/ember-admin/app/routes/site.js b/apps/ember-admin/app/routes/site.js deleted file mode 100644 index e95d427b717..00000000000 --- a/apps/ember-admin/app/routes/site.js +++ /dev/null @@ -1,13 +0,0 @@ -import AuthenticatedRoute from 'ghost-admin/routes/authenticated'; - -export default class SiteRoute extends AuthenticatedRoute { - model() { - return (new Date()).valueOf(); - } - - buildRouteInfoMetadata() { - return { - titleToken: 'Site' - }; - } -} diff --git a/apps/ember-admin/app/services/migrate.js b/apps/ember-admin/app/services/migrate.js deleted file mode 100644 index 7c21e5f830d..00000000000 --- a/apps/ember-admin/app/services/migrate.js +++ /dev/null @@ -1,135 +0,0 @@ -import Service, {inject as service} from '@ember/service'; -import config from 'ghost-admin/config/environment'; -import {tracked} from '@glimmer/tracking'; - -export default class MigrateService extends Service { - @service ajax; - @service billing; - @service feature; - @service router; - @service ghostPaths; - @service settings; - - migrateUrl = 'https://migrate.ghost.org'; - migrateRouteRoot = '#/migrate'; - - @tracked migrateWindowOpen = false; - @tracked siteData = null; - @tracked previousRoute = null; - @tracked isIframeTransition = false; - @tracked platform = null; - - get apiUrl() { - const origin = window.location.origin; - const subdir = this.ghostPaths.subdir; - const rootURL = this.router.rootURL; - let url = this.ghostPaths.url.join(origin, subdir, rootURL); - url = url.replace(/\/$/, ''); // Strips the trailing slash - return url; - } - - async apiKey() { - const ghostIntegrationsUrl = this.ghostPaths.url.api('integrations') + '?include=api_keys'; - return this.ajax.request(ghostIntegrationsUrl).then(async (response) => { - const ssmIntegration = response.integrations.find(r => r.slug === 'self-serve-migration'); - - const key = ssmIntegration.api_keys[0].secret; - - return key; - }).catch((error) => { - throw error; - }); - } - - async postMessagePayload() { - const theKey = await this.apiKey(); - const theOwner = await this.billing.getOwnerUser(); - - const payload = { - apiUrl: this.apiUrl, - apiKey: theKey, - stripe: this.isStripeConnected, - csvContentImporter: this.isCsvContentImporterEnabled, - ghostVersion: this.ghostVersion, - ownerEmail: theOwner.email - }; - - return payload; - } - - get isStripeConnected() { - return (this.settings.stripeConnectAccountId && this.settings.stripeConnectPublishableKey && this.settings.stripeConnectLivemode) ? true : false; - } - - get isCsvContentImporterEnabled() { - return this.feature.csvContentImporter ? true : false; - } - - get ghostVersion() { - return config.APP.version; - } - - constructor() { - super(...arguments); - } - - getIframeURL() { - let url = this.migrateUrl; - const params = this.router.currentRoute.params; - if (params.platform) { - url = url + '?platform=' + params.platform; - } - - return url; - } - - // Sends a route update to a child route in the migrate app, because we can't control - // navigating to it otherwise - sendRouteUpdate(route) { - this.getMigrateIframe().contentWindow.postMessage({ - query: 'routeUpdate', - response: route - }, this.migrateUrl); - } - - // Controls migrate window modal visibility and sync of the URL visible in browser - // and the URL opened on the iframe. It is responsible to non user triggered iframe opening, - // for example: by entering "/migrate" route in the URL or using history navigation (back and forward) - toggleMigrateWindow(value) { - if (this.migrateWindowOpen && value) { - // don't attempt to open again - return; - } - this.migrateWindowOpen = value; - } - - // Controls navigation to migrate window modal which is triggered from the application UI. - // For example: pressing "View migrate" link in navigation menu. It's main side effect is - // remembering the route from which the action has been triggered - "previousRoute" so it - // could be reused when closing the migrate window - openMigrateWindow(currentRoute, childRoute) { - if (this.migrateWindowOpen) { - // don't attempt to open again - return; - } - - this.previousRoute = currentRoute; - - // Ensures correct "getIframeURL" calculation when syncing iframe location - // in toggleMigrateWindow - window.location.hash = childRoute || '/migrate'; - - this.router.transitionTo(childRoute || '/migrate'); - this.toggleMigrateWindow(true); - } - - closeMigrateWindow() { - window.location.hash = '/settings/migration'; - this.router.transitionTo('/settings/migration'); - this.toggleMigrateWindow(false); - } - - getMigrateIframe() { - return document.getElementById('migrate-frame'); - } -} diff --git a/apps/ember-admin/app/styles/app.css b/apps/ember-admin/app/styles/app.css index 597b9218e85..eb5a7695de8 100644 --- a/apps/ember-admin/app/styles/app.css +++ b/apps/ember-admin/app/styles/app.css @@ -64,7 +64,6 @@ @import "layouts/post-preview.css"; @import "layouts/tiers.css"; @import "layouts/mentions.css"; -@import "layouts/migrate.css"; /* Suppress transitions/animations during a light/dark mode swap so the diff --git a/apps/ember-admin/app/styles/components/browser-preview.css b/apps/ember-admin/app/styles/components/browser-preview.css index 4c8e41496db..de13b7da489 100644 --- a/apps/ember-admin/app/styles/components/browser-preview.css +++ b/apps/ember-admin/app/styles/components/browser-preview.css @@ -22,29 +22,6 @@ border-radius: 8px 8px 0 0; } -.gh-browserpreview-iframecontainer .site-frame { - border-bottom-left-radius: 3px; - border-bottom-right-radius: 3px; -} - -@media (max-width: 1600px) { - .gh-browserpreview-iframecontainer .site-frame iframe { - width: 130%; - height: 130%; - transform: scale(calc(100 / 130)); - transform-origin: 0 0; - } -} - -@media (min-width: 1601px) and (max-width: 1920px) { - .gh-browserpreview-iframecontainer .site-frame iframe { - width: 110%; - height: 110%; - transform: scale(calc(100 / 110)); - transform-origin: 0 0; - } -} - .gh-browserpreview-browser { background: var(--whitegrey-l1); border-top-left-radius: 3px; diff --git a/apps/ember-admin/app/styles/layouts/migrate.css b/apps/ember-admin/app/styles/layouts/migrate.css deleted file mode 100644 index 0418653c367..00000000000 --- a/apps/ember-admin/app/styles/layouts/migrate.css +++ /dev/null @@ -1,57 +0,0 @@ -#ember-app.migration-app-open { - position: relative; - z-index: 10; -} - -.gh-migrate { - position: absolute; - top: 0; - left: 0; - height: 100%; - width: 100%; - z-index: 9999; - background: var(--main-bg-color); -} - -.gh-migrate-container { - position: relative; - height: 100%; - width: 100%; -} - -.gh-migrate-fullscreen-container { - position: relative; - position: fixed; - top: 0; - right: 0; - bottom: 0; - left: 0; - z-index: 10000; - height: 100vh; - background: var(--main-bg-color); - overflow: hidden; -} - -.gh-migrate .migrate-frame { - position: absolute; - top: 0; - right: 0; - bottom: 0; - left: 0; - width: 100%; - height: 100%; - border: none; - z-index: 10; - transform: translate3d(0, 0, 0); -} - -.gh-migrate-close { - /* Other styles are classes form the design system */ - z-index: 10000; /* One more than the iframe wrapper */ -} - -.gh-migrate-close a svg { - display: block; - width: 2rem; - height: 2rem; -} diff --git a/apps/ember-admin/app/styles/patterns/global.css b/apps/ember-admin/app/styles/patterns/global.css index c6af0ace8dc..e8a925812ff 100644 --- a/apps/ember-admin/app/styles/patterns/global.css +++ b/apps/ember-admin/app/styles/patterns/global.css @@ -814,15 +814,3 @@ input[type="image"] { .liquid-container.show-overflow.liquid-animating .liquid-child { overflow: hidden; } - -.site-frame { - position: absolute; - top: 0; - right: 0; - bottom: 0; - left: 0; - width: 100%; - height: 100%; - border: none; - transform: translate3d(0, 0, 0); -} diff --git a/apps/ember-admin/app/templates/migrate.hbs b/apps/ember-admin/app/templates/migrate.hbs deleted file mode 100644 index f565f1ec900..00000000000 --- a/apps/ember-admin/app/templates/migrate.hbs +++ /dev/null @@ -1,9 +0,0 @@ -
-
- - {{svg-jar "close"}} - -
- - -
\ No newline at end of file diff --git a/apps/ember-admin/app/templates/site.hbs b/apps/ember-admin/app/templates/site.hbs deleted file mode 100644 index 2170561630d..00000000000 --- a/apps/ember-admin/app/templates/site.hbs +++ /dev/null @@ -1 +0,0 @@ - diff --git a/apps/ember-admin/tests/integration/components/gh-site-iframe-test.js b/apps/ember-admin/tests/integration/components/gh-site-iframe-test.js deleted file mode 100644 index 4bb24b6ce33..00000000000 --- a/apps/ember-admin/tests/integration/components/gh-site-iframe-test.js +++ /dev/null @@ -1,23 +0,0 @@ -import hbs from 'htmlbars-inline-precompile'; -import {describe, it} from 'mocha'; -import {expect} from 'chai'; -import {find, render} from '@ember/test-helpers'; -import {setupRenderingTest} from 'ember-mocha'; - -describe('Integration: Component: gh-site-iframe', function () { - setupRenderingTest(); - - beforeEach(function () { - this.owner.register('config:main', { - blogUrl: 'http://localhost:2368' - }, {instantiate: false}); - }); - - it('forwards the View site preview marker to the iframe element', async function () { - await render(hbs``); - - const iframe = find('iframe'); - expect(iframe).to.have.attribute('data-view-site-preview'); - expect(iframe).to.have.class('site-frame'); - }); -}); diff --git a/apps/ember-admin/tests/unit/services/migrate-test.js b/apps/ember-admin/tests/unit/services/migrate-test.js deleted file mode 100644 index 8e48470f6dc..00000000000 --- a/apps/ember-admin/tests/unit/services/migrate-test.js +++ /dev/null @@ -1,74 +0,0 @@ -import Service from '@ember/service'; -import sinon from 'sinon'; -import {describe, it} from 'mocha'; -import {expect} from 'chai'; -import {setupTest} from 'ember-mocha'; - -const isValidUrl = (str) => { - try { - new URL(str); return true; - } catch { - return false; - } -}; - -describe('Unit: Service: migrate', function () { - setupTest(); - - let migrateService; - - beforeEach(function () { - migrateService = this.owner.lookup('service:migrate'); - }); - - it('exists', function () { - expect(migrateService).to.be.ok; - }); - - it('can generate valid payload', async function () { - sinon.stub(migrateService, 'apiKey').resolves('abcd:1234'); - - this.owner.register('service:billing', Service.extend({ - getOwnerUser: () => { - return { - email: 'name@example.com' - }; - } - })); - - this.owner.register('service:feature', Service.extend({ - csvContentImporter: false - })); - - const payload = await migrateService.postMessagePayload(); - - expect(payload).to.be.an('object').that.has.all.keys('apiUrl', 'apiKey', 'stripe', 'csvContentImporter', 'ghostVersion', 'ownerEmail'); - expect(isValidUrl(payload.apiUrl)).to.be.true; - expect(payload.apiUrl.endsWith('/ghost')).to.be.true; - expect(payload.apiKey).to.equal('abcd:1234'); - expect(payload.stripe).to.be.false; - expect(payload.csvContentImporter).to.be.false; - expect(payload.ghostVersion).to.be.string; - expect(payload.ownerEmail).to.equal('name@example.com'); - }); - - it('reports csvContentImporter as enabled when the labs flag is on', async function () { - sinon.stub(migrateService, 'apiKey').resolves('abcd:1234'); - - this.owner.register('service:billing', Service.extend({ - getOwnerUser: () => { - return { - email: 'name@example.com' - }; - } - })); - - this.owner.register('service:feature', Service.extend({ - csvContentImporter: true - })); - - const payload = await migrateService.postMessagePayload(); - - expect(payload.csvContentImporter).to.be.true; - }); -}); diff --git a/e2e/helpers/pages/admin/index.ts b/e2e/helpers/pages/admin/index.ts index 596ac2f32aa..c17d7a55152 100644 --- a/e2e/helpers/pages/admin/index.ts +++ b/e2e/helpers/pages/admin/index.ts @@ -13,5 +13,6 @@ export * from './posts'; export * from './tags'; export * from './sidebar'; export * from './site-page'; +export * from './migrate-page'; export * from './billing'; export * from './comments'; diff --git a/e2e/helpers/pages/admin/migrate-page.ts b/e2e/helpers/pages/admin/migrate-page.ts new file mode 100644 index 00000000000..10b1248bd8f --- /dev/null +++ b/e2e/helpers/pages/admin/migrate-page.ts @@ -0,0 +1,17 @@ +import { AdminPage } from './admin-page'; +import { FrameLocator, Locator, Page } from '@playwright/test'; +import { closeMigrateButton, migrateFrame } from '@tryghost/test-data/selectors/migrate'; + +export class MigratePage extends AdminPage { + readonly migrationAppFrame: Locator; + readonly migrationApp: FrameLocator; + readonly closeButton: Locator; + + constructor(page: Page) { + super(page); + this.pageUrl = '/ghost/#/migrate'; + this.migrationAppFrame = page.getByTitle(migrateFrame, { exact: true }); + this.migrationApp = this.migrationAppFrame.contentFrame(); + this.closeButton = page.getByRole('button', { name: closeMigrateButton, exact: true }); + } +} diff --git a/e2e/helpers/pages/admin/site-page.ts b/e2e/helpers/pages/admin/site-page.ts index c14f34c6b7d..290099cde33 100644 --- a/e2e/helpers/pages/admin/site-page.ts +++ b/e2e/helpers/pages/admin/site-page.ts @@ -1,5 +1,6 @@ import { AdminPage } from './admin-page'; import { Locator, Page } from '@playwright/test'; +import { sitePreviewFrame } from '@tryghost/test-data/selectors/view-site'; export class SitePage extends AdminPage { readonly sitePreview: Locator; @@ -7,7 +8,7 @@ export class SitePage extends AdminPage { constructor(page: Page) { super(page); this.pageUrl = '/ghost/#/site'; - this.sitePreview = page.getByTitle('Site preview'); + this.sitePreview = page.getByTitle(sitePreviewFrame); } async waitForPageToFullyLoad(): Promise { diff --git a/e2e/tests/admin/iframe-routes/migration-app-handoff.test.ts b/e2e/tests/admin/iframe-routes/migration-app-handoff.test.ts new file mode 100644 index 00000000000..f5762862366 --- /dev/null +++ b/e2e/tests/admin/iframe-routes/migration-app-handoff.test.ts @@ -0,0 +1,62 @@ +import { MigratePage } from '@/admin-pages'; +import { expect, test } from '@/helpers/playwright'; + +const MIGRATION_APP_ORIGIN = 'https://migrate.ghost.org'; + +// Stands in for the migration app: asks Admin for its credentials, shows what +// it received, and can send Admin back to the migration settings. +const migrationAppStandIn = ` + 

+  
+  
+`;
+
+interface IntegrationsResponse {
+  integrations: Array<{ slug: string; api_keys: Array<{ secret: string }> }>;
+}
+
+test.describe('Ghost Admin - Migration app handoff', () => {
+  test('gives the migration app its credentials and follows it back to settings', async ({
+    page,
+    baseURL,
+    ghostAccountOwner,
+  }) => {
+    const response = await page.request.get('/ghost/api/admin/integrations/', {
+      params: { include: 'api_keys' },
+    });
+    const { integrations } = (await response.json()) as IntegrationsResponse;
+    const migrationKey = integrations.find(({ slug }) => slug === 'self-serve-migration')
+      ?.api_keys[0].secret;
+    await page.route(
+      (url) => url.origin === MIGRATION_APP_ORIGIN,
+      (route) => route.fulfill({ contentType: 'text/html', body: migrationAppStandIn }),
+    );
+
+    const migratePage = new MigratePage(page);
+    await migratePage.goto('/ghost/#/migrate/substack');
+
+    const initialData = migratePage.migrationApp.locator('#initial-data');
+    await expect(initialData).not.toBeEmpty();
+    await expect(migratePage.closeButton).toBeVisible();
+    expect(JSON.parse((await initialData.textContent()) ?? '')).toEqual({
+      apiUrl: `${new URL(baseURL ?? '').origin}/ghost`,
+      apiKey: migrationKey,
+      stripe: false,
+      csvContentImporter: false,
+      ghostVersion: expect.stringMatching(/^\d+\.\d+$/),
+      ownerEmail: ghostAccountOwner.email,
+    });
+
+    await migratePage.migrationApp.getByRole('button', { name: 'Back to Ghost' }).click();
+
+    await expect(page).toHaveURL(/\/ghost\/#\/settings\/migration\/?$/);
+    await expect(migratePage.migrationAppFrame).toBeHidden();
+  });
+});
diff --git a/e2e/tests/admin/iframe-routes/view-site.test.ts b/e2e/tests/admin/iframe-routes/view-site.test.ts
new file mode 100644
index 00000000000..fd4ec946f94
--- /dev/null
+++ b/e2e/tests/admin/iframe-routes/view-site.test.ts
@@ -0,0 +1,15 @@
+import { SidebarPage, SitePage } from '@/admin-pages';
+import { expect, test } from '@/helpers/playwright';
+
+test.describe('Ghost Admin - View site', () => {
+  test('shows the site homepage inside Admin', async ({ page }) => {
+    const sidebar = new SidebarPage(page);
+    await sidebar.goto('/ghost/#/analytics');
+
+    await sidebar.getNavLink('View site').click();
+
+    const sitePage = new SitePage(page);
+    await sitePage.waitForPageToFullyLoad();
+    await expect(sitePage.sitePreview.contentFrame().locator('body.home-template')).toBeVisible();
+  });
+});
diff --git a/packages/testing/test-data/src/selectors/migrate.ts b/packages/testing/test-data/src/selectors/migrate.ts
new file mode 100644
index 00000000000..970ad08f9ad
--- /dev/null
+++ b/packages/testing/test-data/src/selectors/migrate.ts
@@ -0,0 +1,8 @@
+/**
+ * Migration screen selector strings, consumed by the admin screen helpers and
+ * the e2e page objects. Source of truth: apps/admin/src/migrate.
+ */
+
+// accessible names
+export const migrateFrame = 'Migrate';
+export const closeMigrateButton = 'Close';
diff --git a/packages/testing/test-data/src/selectors/view-site.ts b/packages/testing/test-data/src/selectors/view-site.ts
new file mode 100644
index 00000000000..57084241239
--- /dev/null
+++ b/packages/testing/test-data/src/selectors/view-site.ts
@@ -0,0 +1,8 @@
+/**
+ * View site selector strings, consumed by the admin screen helpers and the
+ * e2e page objects. Source of truth: apps/admin/src/view-site.
+ */
+
+// accessible names
+export const sitePreviewFrame = 'Site preview';
+export const viewSiteNavLink = 'View site';

From f2bff7ff1009b26d65c6b658288b5ea3671d4b7a Mon Sep 17 00:00:00 2001
From: Steve Larson <9larsons@gmail.com>
Date: Mon, 28 Sep 2026 20:13:50 +0200
Subject: [PATCH 08/21] Improved Admin acceptance test output (#31038)

no ref

Tidied up test output so that passing tests don't leave 1ks of lines.
---
 apps/admin/test-utils/acceptance/README.md | 1 +
 apps/admin/vitest.acceptance.config.ts     | 8 ++++++++
 2 files changed, 9 insertions(+)

diff --git a/apps/admin/test-utils/acceptance/README.md b/apps/admin/test-utils/acceptance/README.md
index e9d66767138..5a64aeaeb38 100644
--- a/apps/admin/test-utils/acceptance/README.md
+++ b/apps/admin/test-utils/acceptance/README.md
@@ -121,6 +121,7 @@ pnpm test:acceptance:watch -- --browser.headless=false   # headed, watch the bro
 
 ## Debugging
 
+- Runs print a final summary and failure details, including console output from failing tests, locally and in CI. To see all console output and individual test results, run `pnpm test:acceptance --silent=false --reporter=verbose` (optionally add a test file path). The same flags work with `test:acceptance:watch`.
 - **Failure screenshots** land in `__screenshots__/` (gitignored) — the fastest way to see what actually rendered.
 - **418 bodies** name the unhandled request and list what is faked.
 - There are **no Playwright traces** in this tier — a spec that needs trace-level debugging belongs in `e2e/`.
diff --git a/apps/admin/vitest.acceptance.config.ts b/apps/admin/vitest.acceptance.config.ts
index 6cea733219b..cb0e35a8341 100644
--- a/apps/admin/vitest.acceptance.config.ts
+++ b/apps/admin/vitest.acceptance.config.ts
@@ -69,6 +69,10 @@ const resetFakeFrameOrigins: BrowserCommand<[]> = async ({ page }) => {
 
 export default defineConfig({
   plugins: [tailwindcss() as PluginOption, react()],
+  server: {
+    // Vitest owns console reporting; Vite forwarding bypasses silent below.
+    forwardConsole: false,
+  },
   // Serves the MSW service worker script; scoped to the test config so it
   // never ends up in the production build's public assets.
   publicDir: './test-utils/acceptance/public',
@@ -83,6 +87,10 @@ export default defineConfig({
   resolve: sharedResolve,
   test: {
     name: 'acceptance',
+    // Print totals and failures, without per-test output that CI expands into
+    // separate lines. Use --silent=false --reporter=verbose to debug.
+    silent: 'passed-only',
+    reporters: process.env.GITHUB_ACTIONS ? ['minimal', 'github-actions'] : ['minimal'],
     include: ['src/**/*.acceptance.test.tsx', 'src/**/*.component.test.tsx'],
     maxWorkers: getWorkerCount(),
     setupFiles: ['./test-utils/acceptance/setup.ts'],

From 7cde4c342336d3ebdd604d21172d3e256588a675 Mon Sep 17 00:00:00 2001
From: Austin Burdine 
Date: Mon, 28 Sep 2026 14:43:26 -0400
Subject: [PATCH 09/21] =?UTF-8?q?=F0=9F=90=9B=20Fixed=20crashes=20when=20a?=
 =?UTF-8?q?=20subscription's=20member=20or=20paid=20tier=20is=20missing=20?=
 =?UTF-8?q?(#31047)?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

no ref

The Stripe subscription methods in the member repository dereferenced the
result of `findOne` without checking it, so an unknown member id or email
produced a 500 instead of a 404. They now throw NotFoundError, matching the
rest of the repository.

In Portal, a `data-members-plan="monthly|yearly"` button on a site with no
available paid tier threw before the click handler was re-attached, leaving
the button dead with no error shown. The request now goes through without a
tier, so the server's 400 is surfaced and the button recovers.
---
 apps/portal/src/utils/helpers.js              |  4 +-
 apps/portal/test/data-attributes.test.jsx     | 21 ++++++++++
 .../repositories/member-repository.js         | 20 ++++++++++
 .../repositories/member-repository.test.js    | 39 +++++++++++++++++++
 4 files changed, 82 insertions(+), 2 deletions(-)

diff --git a/apps/portal/src/utils/helpers.js b/apps/portal/src/utils/helpers.js
index 30332001ae6..a8f096c8e29 100644
--- a/apps/portal/src/utils/helpers.js
+++ b/apps/portal/src/utils/helpers.js
@@ -218,13 +218,13 @@ export function getCheckoutSessionDataFromPlanAttribute(site, plan) {
   if (plan === 'monthly') {
     return {
       cadence: 'month',
-      tierId: defaultTier.id,
+      tierId: defaultTier?.id,
     };
   }
   if (plan === 'yearly') {
     return {
       cadence: 'year',
-      tierId: defaultTier.id,
+      tierId: defaultTier?.id,
     };
   }
   return {
diff --git a/apps/portal/test/data-attributes.test.jsx b/apps/portal/test/data-attributes.test.jsx
index d76c1e4539b..a10a6ee2a8a 100644
--- a/apps/portal/test/data-attributes.test.jsx
+++ b/apps/portal/test/data-attributes.test.jsx
@@ -425,6 +425,27 @@ describe('Member Data attributes:', () => {
         },
       );
     });
+
+    test('shows an error instead of crashing when no paid tier is available', async () => {
+      const { event, errorEl, siteUrl, member, element } = getMockData();
+      const site = FixturesSite.singleTier.onlyFreePlan;
+      const clickHandler = () => {};
+      element.addEventListener = vi.fn();
+
+      window.fetch.mockImplementation((url) => {
+        if (url.includes('api/session')) {
+          return Promise.resolve({ ok: true, text: async () => 'session-identity' });
+        }
+        return Promise.resolve({ ok: false });
+      });
+
+      await planClickHandler({ event, errorEl, siteUrl, clickHandler, site, member, el: element });
+
+      const [, checkoutOptions] = window.fetch.mock.calls[1];
+      expect(JSON.parse(checkoutOptions.body)).not.toHaveProperty('tierId');
+      expect(errorEl.innerText).toBe('Could not create Stripe checkout session');
+      expect(element.addEventListener).toHaveBeenCalledWith('click', clickHandler);
+    });
   });
 
   describe('data-members-manage-billing', () => {
diff --git a/ghost/core/core/server/services/members/members-api/repositories/member-repository.js b/ghost/core/core/server/services/members/members-api/repositories/member-repository.js
index 3538654b608..99331d6cb52 100644
--- a/ghost/core/core/server/services/members/members-api/repositories/member-repository.js
+++ b/ghost/core/core/server/services/members/members-api/repositories/member-repository.js
@@ -1247,6 +1247,10 @@ module.exports = class MemberRepository {
       { ...options, forUpdate: true },
     );
 
+    if (!memberModel) {
+      throw new errors.NotFoundError({ message: tpl(messages.memberNotFound, { id: data.id }) });
+    }
+
     const memberStripeCustomerModel = await memberModel
       .related('stripeCustomers')
       .query({
@@ -2038,6 +2042,10 @@ module.exports = class MemberRepository {
       email: data.email,
     });
 
+    if (!member) {
+      throw new errors.NotFoundError({ message: tpl(messages.memberNotFound, { id: data.email }) });
+    }
+
     const subscription = await member
       .related('stripeSubscriptions')
       .query({
@@ -2079,6 +2087,12 @@ module.exports = class MemberRepository {
 
     const member = await this._Member.findOne(findQuery);
 
+    if (!member) {
+      throw new errors.NotFoundError({
+        message: tpl(messages.memberNotFound, { id: data.id || data.email }),
+      });
+    }
+
     const subscription = await member
       .related('stripeSubscriptions')
       .query({
@@ -2138,6 +2152,12 @@ module.exports = class MemberRepository {
 
     const member = await this._Member.findOne(findQuery);
 
+    if (!member) {
+      throw new errors.NotFoundError({
+        message: tpl(messages.memberNotFound, { id: data.id || data.email }),
+      });
+    }
+
     const subscriptionModel = await member
       .related('stripeSubscriptions')
       .query({
diff --git a/ghost/core/test/unit/server/services/members/members-api/repositories/member-repository.test.js b/ghost/core/test/unit/server/services/members/members-api/repositories/member-repository.test.js
index 1f8fbb8e4d0..4855eba8e23 100644
--- a/ghost/core/test/unit/server/services/members/members-api/repositories/member-repository.test.js
+++ b/ghost/core/test/unit/server/services/members/members-api/repositories/member-repository.test.js
@@ -531,6 +531,45 @@ describe('MemberRepository', function () {
     });
   });
 
+  describe('subscription methods with a missing member', function () {
+    let repo;
+
+    beforeEach(function () {
+      Member.findOne = sinon.stub().resolves(null);
+      repo = buildRepo({ stripeAPIService: { configured: true } });
+    });
+
+    const subscription = { id: 'sub_123', subscription_id: 'sub_123', customer: 'cus_123' };
+
+    it('linkSubscription throws NotFoundError', async function () {
+      await assert.rejects(
+        repo.linkSubscription({ id: 'missing', subscription }, { transacting: {} }),
+        errors.NotFoundError,
+      );
+    });
+
+    it('getSubscription throws NotFoundError', async function () {
+      await assert.rejects(
+        repo.getSubscription({ email: 'missing@example.com', subscription }),
+        errors.NotFoundError,
+      );
+    });
+
+    it('cancelSubscription throws NotFoundError', async function () {
+      await assert.rejects(
+        repo.cancelSubscription({ id: 'missing', subscription }),
+        errors.NotFoundError,
+      );
+    });
+
+    it('updateSubscription throws NotFoundError', async function () {
+      await assert.rejects(
+        repo.updateSubscription({ email: 'missing@example.com', subscription }),
+        errors.NotFoundError,
+      );
+    });
+  });
+
   describe('linkSubscription', function () {
     let subscriptionData;
     let subscriptionCreatedNotifySpy;

From 58f17620ae77e169517789ccf64b7a1960f06f8f Mon Sep 17 00:00:00 2001
From: Evan Hahn 
Date: Mon, 28 Sep 2026 20:47:23 +0200
Subject: [PATCH 10/21] TypeScriptified member attribution unit tests (#31045)

no ref
---
 ...ttribution.test.js => attribution.test.ts} | 60 ++++++++++---
 .../{history.test.js => history.test.ts}      |  5 +-
 ...r.test.js => outbound-link-tagger.test.ts} |  9 +-
 ...or.test.js => referrer-translator.test.ts} | 33 ++++++-
 .../{service.test.js => service.test.ts}      | 52 +++++++----
 ...nslator.test.js => url-translator.test.ts} | 87 ++++++++++++++-----
 6 files changed, 183 insertions(+), 63 deletions(-)
 rename ghost/core/test/unit/server/services/member-attribution/{attribution.test.js => attribution.test.ts} (82%)
 rename ghost/core/test/unit/server/services/member-attribution/{history.test.js => history.test.ts} (92%)
 rename ghost/core/test/unit/server/services/member-attribution/{outbound-link-tagger.test.js => outbound-link-tagger.test.ts} (97%)
 rename ghost/core/test/unit/server/services/member-attribution/{referrer-translator.test.js => referrer-translator.test.ts} (94%)
 rename ghost/core/test/unit/server/services/member-attribution/{service.test.js => service.test.ts} (90%)
 rename ghost/core/test/unit/server/services/member-attribution/{url-translator.test.js => url-translator.test.ts} (81%)

diff --git a/ghost/core/test/unit/server/services/member-attribution/attribution.test.js b/ghost/core/test/unit/server/services/member-attribution/attribution.test.ts
similarity index 82%
rename from ghost/core/test/unit/server/services/member-attribution/attribution.test.js
rename to ghost/core/test/unit/server/services/member-attribution/attribution.test.ts
index fd8b3818490..2356e6521dd 100644
--- a/ghost/core/test/unit/server/services/member-attribution/attribution.test.js
+++ b/ghost/core/test/unit/server/services/member-attribution/attribution.test.ts
@@ -1,17 +1,51 @@
-const { assertObjectMatches } = require('../../../../utils/assertions');
+import { assertObjectMatches } from '../../../../utils/assertions';
+// @ts-expect-error JavaScript module has no type declarations
+import UrlHistory from '../../../../../core/server/services/member-attribution/url-history';
+// @ts-expect-error JavaScript module has no type declarations
+import AttributionBuilder from '../../../../../core/server/services/member-attribution/attribution-builder';
 
-const UrlHistory = require('../../../../../core/server/services/member-attribution/url-history');
-const AttributionBuilder = require('../../../../../core/server/services/member-attribution/attribution-builder');
+type HistoryItem = {
+  id?: string;
+  path?: string;
+  time: number;
+  type?: string;
+};
+
+type UrlHistoryLike = Iterable & { length: number };
+type AttributionData = {
+  id?: string | number | null;
+  type?: string | null;
+  url?: string | null;
+};
+type AttributionLike = {
+  fetchResource(): Promise>;
+};
+type AttributionBuilderLike = {
+  build(data: AttributionData): AttributionLike;
+  getAttribution(history: UrlHistoryLike): Promise>;
+};
+type ResourceModel = {
+  id: string;
+  get(property: string): string | undefined;
+};
+type UrlTranslatorMock = {
+  getResourceDetails(item: HistoryItem): AttributionData | null;
+  getResourceById(id: string, type: string): ResourceModel | null;
+  getUrlTitle(url: string): string;
+  getResourceUrl(): string;
+  relativeToAbsolute(path: string): string;
+  stripSubdirectoryFromPath(path: string): string;
+};
 
 describe('AttributionBuilder', function () {
-  let attributionBuilder;
-  let urlTranslator;
-  let now;
+  let attributionBuilder: AttributionBuilderLike;
+  let urlTranslator: UrlTranslatorMock;
+  let now: number;
 
   beforeAll(function () {
     now = Date.now();
     urlTranslator = {
-      getResourceDetails(item) {
+      getResourceDetails(item: HistoryItem) {
         if (!item.path) {
           if (item.id === 'invalid') {
             return null;
@@ -46,13 +80,13 @@ describe('AttributionBuilder', function () {
           url: path,
         };
       },
-      getResourceById(id, type) {
+      getResourceById(id: string, type: string) {
         if (id === 'invalid') {
           return null;
         }
         return {
           id,
-          get(prop) {
+          get(prop: string) {
             if (prop === 'title' && type === 'author') {
               // Simulate an author doesn't have a title
               return undefined;
@@ -65,16 +99,16 @@ describe('AttributionBuilder', function () {
           },
         };
       },
-      getUrlTitle(url) {
+      getUrlTitle(url: string) {
         return url;
       },
       getResourceUrl() {
         return 'https://absolute/dir/path';
       },
-      relativeToAbsolute(path) {
+      relativeToAbsolute(path: string) {
         return 'https://absolute/dir' + path;
       },
-      stripSubdirectoryFromPath(path) {
+      stripSubdirectoryFromPath(path: string) {
         if (path.startsWith('/dir/')) {
           return path.substring('/dir/'.length - 1);
         }
@@ -84,7 +118,7 @@ describe('AttributionBuilder', function () {
     attributionBuilder = new AttributionBuilder({
       urlTranslator,
       referrerTranslator: {
-        getReferrerDetails(history) {
+        getReferrerDetails(history: UrlHistoryLike) {
           if (history) {
             return {
               referrerSource: 'Ghost Explore',
diff --git a/ghost/core/test/unit/server/services/member-attribution/history.test.js b/ghost/core/test/unit/server/services/member-attribution/history.test.ts
similarity index 92%
rename from ghost/core/test/unit/server/services/member-attribution/history.test.js
rename to ghost/core/test/unit/server/services/member-attribution/history.test.ts
index b1d31682f3d..9587b1a8e89 100644
--- a/ghost/core/test/unit/server/services/member-attribution/history.test.js
+++ b/ghost/core/test/unit/server/services/member-attribution/history.test.ts
@@ -1,6 +1,7 @@
-const assert = require('node:assert/strict');
+import assert from 'node:assert/strict';
 
-const UrlHistory = require('../../../../../core/server/services/member-attribution/url-history');
+// @ts-expect-error JavaScript module has no type declarations
+import UrlHistory from '../../../../../core/server/services/member-attribution/url-history';
 
 describe('UrlHistory', function () {
   it('sets history to empty array if invalid', function () {
diff --git a/ghost/core/test/unit/server/services/member-attribution/outbound-link-tagger.test.js b/ghost/core/test/unit/server/services/member-attribution/outbound-link-tagger.test.ts
similarity index 97%
rename from ghost/core/test/unit/server/services/member-attribution/outbound-link-tagger.test.js
rename to ghost/core/test/unit/server/services/member-attribution/outbound-link-tagger.test.ts
index 8e6fec70f48..e3574107484 100644
--- a/ghost/core/test/unit/server/services/member-attribution/outbound-link-tagger.test.js
+++ b/ghost/core/test/unit/server/services/member-attribution/outbound-link-tagger.test.ts
@@ -1,6 +1,7 @@
-const assert = require('node:assert/strict');
+import assert from 'node:assert/strict';
 
-const OutboundLinkTagger = require('../../../../../core/server/services/member-attribution/outbound-link-tagger');
+// @ts-expect-error JavaScript module has no type declarations
+import OutboundLinkTagger from '../../../../../core/server/services/member-attribution/outbound-link-tagger';
 
 describe('OutboundLinkTagger', function () {
   describe('Constructor', function () {
@@ -40,7 +41,7 @@ describe('OutboundLinkTagger', function () {
       const url = new URL('https://example.com/');
       const newsletterName = 'used newsletter name';
       const newsletter = {
-        get: (t) => {
+        get: (t: string) => {
           if (t === 'name') {
             return newsletterName;
           }
@@ -63,7 +64,7 @@ describe('OutboundLinkTagger', function () {
       const url = new URL('https://example.com/');
       const newsletterName = 'Weekly newsletter';
       const newsletter = {
-        get: (t) => {
+        get: (t: string) => {
           if (t === 'name') {
             return newsletterName;
           }
diff --git a/ghost/core/test/unit/server/services/member-attribution/referrer-translator.test.js b/ghost/core/test/unit/server/services/member-attribution/referrer-translator.test.ts
similarity index 94%
rename from ghost/core/test/unit/server/services/member-attribution/referrer-translator.test.js
rename to ghost/core/test/unit/server/services/member-attribution/referrer-translator.test.ts
index f67dd066965..18f0fecccf5 100644
--- a/ghost/core/test/unit/server/services/member-attribution/referrer-translator.test.js
+++ b/ghost/core/test/unit/server/services/member-attribution/referrer-translator.test.ts
@@ -1,6 +1,33 @@
-const assert = require('node:assert/strict');
+import assert from 'node:assert/strict';
 
-const ReferrerTranslator = require('../../../../../core/server/services/member-attribution/referrer-translator');
+// @ts-expect-error JavaScript module has no type declarations
+import ReferrerTranslator from '../../../../../core/server/services/member-attribution/referrer-translator';
+
+type ReferrerHistoryItem = {
+  referrerSource?: string | null;
+  referrerMedium?: string | null;
+  referrerUrl?: string | null;
+  utmSource?: string;
+  utmMedium?: string;
+  utmCampaign?: string;
+  utmTerm?: string;
+  utmContent?: string;
+};
+
+type ReferrerData = {
+  referrerSource?: string | null;
+  referrerMedium?: string | null;
+  referrerUrl?: string | null;
+  utmSource?: string | null;
+  utmMedium?: string | null;
+  utmCampaign?: string | null;
+  utmTerm?: string | null;
+  utmContent?: string | null;
+};
+
+type ReferrerTranslatorLike = {
+  getReferrerDetails(history: ReferrerHistoryItem[]): ReferrerData | null;
+};
 
 describe('ReferrerTranslator', function () {
   describe('Constructor', function () {
@@ -10,7 +37,7 @@ describe('ReferrerTranslator', function () {
   });
 
   describe('getReferrerDetails', function () {
-    let translator;
+    let translator: ReferrerTranslatorLike;
     beforeAll(function () {
       translator = new ReferrerTranslator({
         siteUrl: 'https://example.com',
diff --git a/ghost/core/test/unit/server/services/member-attribution/service.test.js b/ghost/core/test/unit/server/services/member-attribution/service.test.ts
similarity index 90%
rename from ghost/core/test/unit/server/services/member-attribution/service.test.js
rename to ghost/core/test/unit/server/services/member-attribution/service.test.ts
index 3bf8b1d800f..c91e47d8870 100644
--- a/ghost/core/test/unit/server/services/member-attribution/service.test.js
+++ b/ghost/core/test/unit/server/services/member-attribution/service.test.ts
@@ -1,10 +1,28 @@
-const assert = require('node:assert/strict');
-
-const MemberAttributionService = require('../../../../../core/server/services/member-attribution/member-attribution-service');
+import assert from 'node:assert/strict';
+
+// @ts-expect-error JavaScript module has no type declarations
+import MemberAttributionService from '../../../../../core/server/services/member-attribution/member-attribution-service';
+
+type AttributionData = {
+  id?: string | null;
+  url?: string | null;
+  type?: string | null;
+  referrerSource?: string | null;
+  referrerMedium?: string | null;
+  referrerUrl?: string | null;
+};
+
+type EventAttributeKey =
+  | 'attribution_id'
+  | 'attribution_url'
+  | 'attribution_type'
+  | 'referrer_source'
+  | 'referrer_medium'
+  | 'referrer_url';
 
 // Mocks a Bookshelf model that is queried via `.where(...).query(...).fetch(...)`,
 // resolving the fetch to the given event (or null).
-function createEventModelMock(event) {
+function createEventModelMock(event: T | null) {
   const chainable = {
     where: () => chainable,
     query: () => chainable,
@@ -98,7 +116,7 @@ describe('MemberAttributionService', function () {
     it('returns null if attribution_type is null', function () {
       const service = new MemberAttributionService({
         attributionBuilder: {
-          build(attribution) {
+          build(attribution: AttributionData) {
             return {
               ...attribution,
               getResource() {
@@ -132,7 +150,7 @@ describe('MemberAttributionService', function () {
     it('returns url attribution types', function () {
       const service = new MemberAttributionService({
         attributionBuilder: {
-          build(attribution) {
+          build(attribution: AttributionData) {
             return {
               ...attribution,
               getResource() {
@@ -148,7 +166,7 @@ describe('MemberAttributionService', function () {
       });
       const model = {
         id: 'event_id',
-        get(name) {
+        get(name: string) {
           if (name === 'attribution_type') {
             return 'url';
           }
@@ -172,7 +190,7 @@ describe('MemberAttributionService', function () {
     it('returns first loaded relation', function () {
       const service = new MemberAttributionService({
         attributionBuilder: {
-          build(attribution) {
+          build(attribution: AttributionData) {
             return {
               ...attribution,
               getResource() {
@@ -188,7 +206,7 @@ describe('MemberAttributionService', function () {
       });
       const model = {
         id: 'event_id',
-        get(name) {
+        get(name: string) {
           if (name === 'attribution_type') {
             return 'user';
           }
@@ -200,7 +218,7 @@ describe('MemberAttributionService', function () {
           }
           return 'test_user_id';
         },
-        related(name) {
+        related(name: string) {
           if (name === 'userAttribution') {
             return {
               id: 'test_user_id',
@@ -225,7 +243,7 @@ describe('MemberAttributionService', function () {
     it('returns attribution from builder with history', async function () {
       const service = new MemberAttributionService({
         attributionBuilder: {
-          getAttribution: async function (history) {
+          getAttribution: async function (history: { length: number }) {
             assert('length' in history);
             return { success: true };
           },
@@ -239,7 +257,7 @@ describe('MemberAttributionService', function () {
     it('returns empty history attribution when tracking disabled', async function () {
       const service = new MemberAttributionService({
         attributionBuilder: {
-          getAttribution: async function (history) {
+          getAttribution: async function (history: { length: number }) {
             assert.equal(history.length, 0);
             return { success: true };
           },
@@ -293,7 +311,7 @@ describe('MemberAttributionService', function () {
       const service = new MemberAttributionService({
         models: {
           MemberCreatedEvent: createEventModelMock({
-            get: function (key) {
+            get: function (key: EventAttributeKey) {
               const values = {
                 attribution_id: 'attr_123',
                 attribution_url: '/test',
@@ -307,7 +325,7 @@ describe('MemberAttributionService', function () {
           }),
         },
         attributionBuilder: {
-          build: function (attribution) {
+          build: function (attribution: AttributionData) {
             return {
               ...attribution,
               fetchResource: async function () {
@@ -347,7 +365,7 @@ describe('MemberAttributionService', function () {
       const service = new MemberAttributionService({
         models: {
           SubscriptionCreatedEvent: createEventModelMock({
-            get: function (key) {
+            get: function (key: EventAttributeKey) {
               const values = {
                 attribution_id: 'attr_123',
                 attribution_url: '/test',
@@ -361,7 +379,7 @@ describe('MemberAttributionService', function () {
           }),
         },
         attributionBuilder: {
-          build: function (attribution) {
+          build: function (attribution: AttributionData) {
             return {
               ...attribution,
               fetchResource: async function () {
@@ -395,7 +413,7 @@ describe('MemberAttributionService', function () {
     it('fetches resource using attribution builder', async function () {
       const service = new MemberAttributionService({
         attributionBuilder: {
-          build: function (attribution) {
+          build: function (attribution: AttributionData) {
             return {
               ...attribution,
               fetchResource: async function () {
diff --git a/ghost/core/test/unit/server/services/member-attribution/url-translator.test.js b/ghost/core/test/unit/server/services/member-attribution/url-translator.test.ts
similarity index 81%
rename from ghost/core/test/unit/server/services/member-attribution/url-translator.test.js
rename to ghost/core/test/unit/server/services/member-attribution/url-translator.test.ts
index 4fef0060664..b6ae4955322 100644
--- a/ghost/core/test/unit/server/services/member-attribution/url-translator.test.js
+++ b/ghost/core/test/unit/server/services/member-attribution/url-translator.test.ts
@@ -1,10 +1,44 @@
-const assert = require('node:assert/strict');
+import assert from 'node:assert/strict';
 
-const UrlTranslator = require('../../../../../core/server/services/member-attribution/url-translator');
+// @ts-expect-error JavaScript module has no type declarations
+import UrlTranslator from '../../../../../core/server/services/member-attribution/url-translator';
+
+type ResourceModel = {
+  id: string;
+  get(property: string): string | undefined;
+};
+type ResourceDetails = {
+  type: string;
+  id: string | null;
+  url: string;
+};
+type ModelForUrl = {
+  get(property: string): string | undefined;
+  toJSON(): Record;
+};
+type UrlTranslatorLike = {
+  getResourceDetails(item: {
+    id?: string;
+    path?: string;
+    type?: string;
+    time: number;
+  }): Promise;
+  getUrlTitle(url: string): string;
+  getTypeAndIdFromPath(path: string): Promise<{ type: string; id: string } | undefined>;
+  getResourceById(id: string, type: string): Promise;
+  getResourceUrl(
+    id: string,
+    type: string,
+    model: ModelForUrl,
+    options: { absolute: boolean },
+  ): string;
+  relativeToAbsolute(path: string): string;
+  stripSubdirectoryFromPath(path: string): string;
+};
 
 const models = {
   Post: {
-    findOne({ id }) {
+    findOne({ id }: { id: string }) {
       if (id === 'invalid') {
         return null;
       }
@@ -12,7 +46,7 @@ const models = {
     },
   },
   User: {
-    findOne({ id }) {
+    findOne({ id }: { id: string }) {
       if (id === 'invalid') {
         return null;
       }
@@ -20,7 +54,7 @@ const models = {
     },
   },
   Tag: {
-    findOne({ id }) {
+    findOne({ id }: { id: string }) {
       if (id === 'invalid') {
         return null;
       }
@@ -37,24 +71,24 @@ describe('UrlTranslator', function () {
   });
 
   describe('getResourceDetails', function () {
-    let translator;
+    let translator: UrlTranslatorLike;
     beforeAll(function () {
       translator = new UrlTranslator({
         urlUtils: {
-          relativeToAbsolute: (t) => {
+          relativeToAbsolute: (t: string) => {
             return 'https://absolute' + t;
           },
-          absoluteToRelative: (t) => {
+          absoluteToRelative: (t: string) => {
             return t
               .replace('https://absolute/with-subdirectory', '')
               .replace('https://absolute', '');
           },
         },
         urlService: {
-          getUrlForResource: (resource) => {
+          getUrlForResource: (resource: { id: string }) => {
             return '/path/' + resource.id;
           },
-          resolveUrl: async (path) => {
+          resolveUrl: async (path: string) => {
             switch (path) {
               case '/path/post':
                 return { type: 'posts', id: 'post' };
@@ -141,7 +175,7 @@ describe('UrlTranslator', function () {
   });
 
   describe('getUrlTitle', function () {
-    let translator;
+    let translator: UrlTranslatorLike;
     beforeAll(function () {
       translator = new UrlTranslator({});
     });
@@ -156,11 +190,11 @@ describe('UrlTranslator', function () {
   });
 
   describe('getTypeAndIdFromPath', function () {
-    let translator;
+    let translator: UrlTranslatorLike;
     beforeAll(function () {
       translator = new UrlTranslator({
         urlService: {
-          resolveUrl: async (path) => {
+          resolveUrl: async (path: string) => {
             switch (path) {
               case '/post':
                 return { type: 'posts', id: 'post' };
@@ -211,7 +245,7 @@ describe('UrlTranslator', function () {
   });
 
   describe('getResourceById', function () {
-    let translator;
+    let translator: UrlTranslatorLike;
     beforeAll(function () {
       translator = new UrlTranslator({
         urlService: {
@@ -223,21 +257,25 @@ describe('UrlTranslator', function () {
 
     it('returns for post', async function () {
       const result = await translator.getResourceById('id', 'post');
+      assert.ok(result);
       assert.equal(result.id, 'post_id');
     });
 
     it('returns for page', async function () {
       const result = await translator.getResourceById('id', 'page');
+      assert.ok(result);
       assert.equal(result.id, 'post_id');
     });
 
     it('returns for tag', async function () {
       const result = await translator.getResourceById('id', 'tag');
+      assert.ok(result);
       assert.equal(result.id, 'tag_id');
     });
 
     it('returns for user', async function () {
       const result = await translator.getResourceById('id', 'author');
+      assert.ok(result);
       assert.equal(result.id, 'user_id');
     });
 
@@ -267,13 +305,13 @@ describe('UrlTranslator', function () {
     // (slug, published_at, primary_tag, ...), so it needs the full resource
     // shape, not just `{id, type}`.
     it("passes the model's plain data (slug, etc.) to the URL service", function () {
-      let captured;
+      let captured: Record | undefined;
       const translator = new UrlTranslator({
         urlUtils: {
-          relativeToAbsolute: (t) => 'https://abs' + t,
+          relativeToAbsolute: (t: string) => 'https://abs' + t,
         },
         urlService: {
-          getUrlForResource: (resource) => {
+          getUrlForResource: (resource: Record) => {
             captured = resource;
             return '/' + resource.slug + '/';
           },
@@ -289,6 +327,7 @@ describe('UrlTranslator', function () {
       const url = translator.getResourceUrl('abc', 'tag', tag, { absolute: false });
 
       assert.equal(url, '/changelog/');
+      assert.ok(captured);
       assert.equal(captured.id, 'abc');
       assert.equal(captured.type, 'tags');
       assert.equal(captured.slug, 'changelog');
@@ -297,7 +336,7 @@ describe('UrlTranslator', function () {
     it('keeps the email-only short-circuit for sent posts', function () {
       const translator = new UrlTranslator({
         urlUtils: {
-          relativeToAbsolute: (t) => 'https://abs' + t,
+          relativeToAbsolute: (t: string) => 'https://abs' + t,
         },
         urlService: {
           getUrlForResource: () => {
@@ -308,7 +347,7 @@ describe('UrlTranslator', function () {
       });
 
       const post = {
-        get(k) {
+        get(k: string) {
           return k === 'status' ? 'sent' : 'uuid-123';
         },
         toJSON: () => ({ id: 'pid', uuid: 'uuid-123', status: 'sent' }),
@@ -322,11 +361,11 @@ describe('UrlTranslator', function () {
   });
 
   describe('relativeToAbsolute', function () {
-    let translator;
+    let translator: UrlTranslatorLike;
     beforeAll(function () {
       translator = new UrlTranslator({
         urlUtils: {
-          relativeToAbsolute: (t) => {
+          relativeToAbsolute: (t: string) => {
             return 'absolute/' + t;
           },
         },
@@ -339,14 +378,14 @@ describe('UrlTranslator', function () {
   });
 
   describe('stripSubdirectoryFromPath', function () {
-    let translator;
+    let translator: UrlTranslatorLike;
     beforeAll(function () {
       translator = new UrlTranslator({
         urlUtils: {
-          relativeToAbsolute: (t) => {
+          relativeToAbsolute: (t: string) => {
             return 'absolute' + t;
           },
-          absoluteToRelative: (t) => {
+          absoluteToRelative: (t: string) => {
             const prefix = 'absolute/dir/';
             if (t.startsWith(prefix)) {
               return t.substring(prefix.length - 1);

From 739456476c82766c5264901d21c3903022097452 Mon Sep 17 00:00:00 2001
From: Evan Hahn 
Date: Mon, 28 Sep 2026 20:47:42 +0200
Subject: [PATCH 11/21] TypeScriptified session controller tests (#31044)

no ref
---
 .../{session.test.js => session.test.ts}      | 54 ++++++++++++-------
 1 file changed, 35 insertions(+), 19 deletions(-)
 rename ghost/core/test/unit/api/canary/{session.test.js => session.test.ts} (84%)

diff --git a/ghost/core/test/unit/api/canary/session.test.js b/ghost/core/test/unit/api/canary/session.test.ts
similarity index 84%
rename from ghost/core/test/unit/api/canary/session.test.js
rename to ghost/core/test/unit/api/canary/session.test.ts
index 63f8300c3a8..c089e9ba87a 100644
--- a/ghost/core/test/unit/api/canary/session.test.js
+++ b/ghost/core/test/unit/api/canary/session.test.ts
@@ -1,11 +1,27 @@
-const assert = require('node:assert/strict');
-const sinon = require('sinon');
-const { UnauthorizedError } = require('@tryghost/errors');
-
-const models = require('../../../../core/server/models');
-
-const sessionController = require('../../../../core/server/api/endpoints/session');
-const sessionServiceMiddleware = require('../../../../core/server/services/auth/session');
+import assert from 'node:assert/strict';
+import sinon from 'sinon';
+import { UnauthorizedError } from '@tryghost/errors';
+
+// @ts-expect-error This module lacks type definitions.
+import models = require('../../../../core/server/models');
+// @ts-expect-error This module lacks type definitions.
+import sessionController from '../../../../core/server/api/endpoints/session';
+// @ts-expect-error This module lacks type definitions.
+import sessionServiceMiddleware = require('../../../../core/server/services/auth/session');
+
+type TestRequest = {
+  brute: {
+    reset: sinon.SinonStub;
+  };
+  user?: unknown;
+  skipVerification?: boolean;
+};
+
+type SessionMiddleware = (
+  req: unknown,
+  res: unknown,
+  next: (error?: unknown) => unknown,
+) => unknown;
 
 describe('Session controller', function () {
   afterEach(function () {
@@ -25,7 +41,7 @@ describe('Session controller', function () {
         () => {
           assert.fail('session.add did not throw');
         },
-        (err) => {
+        (err: unknown) => {
           assert.equal(err instanceof UnauthorizedError, true);
         },
       );
@@ -46,14 +62,14 @@ describe('Session controller', function () {
           () => {
             assert.fail('session.add did not throw');
           },
-          (err) => {
+          (err: unknown) => {
             assert.equal(err instanceof UnauthorizedError, true);
           },
         );
     });
 
     it('it returns a function that calls req.brute.reset, sets req.user and calls createSession if the check works', function () {
-      const fakeReq = {
+      const fakeReq: TestRequest = {
         brute: {
           reset: sinon.stub().callsArg(0),
         },
@@ -73,7 +89,7 @@ describe('Session controller', function () {
             password: 'qu33nRul35',
           },
         })
-        .then((fn) => {
+        .then((fn: SessionMiddleware) => {
           fn(fakeReq, fakeRes, fakeNext);
         })
         .then(function () {
@@ -89,7 +105,7 @@ describe('Session controller', function () {
 
     it('it returns a function that calls req.brute.reset and calls next if reset errors', function () {
       const resetError = new Error();
-      const fakeReq = {
+      const fakeReq: TestRequest = {
         brute: {
           reset: sinon.stub().callsArgWith(0, resetError),
         },
@@ -109,7 +125,7 @@ describe('Session controller', function () {
             password: 'qu33nRul35',
           },
         })
-        .then((fn) => {
+        .then((fn: SessionMiddleware) => {
           fn(fakeReq, fakeRes, fakeNext);
         })
         .then(function () {
@@ -120,7 +136,7 @@ describe('Session controller', function () {
     });
 
     it('it creates a verified session when the user has not logged in before', function () {
-      const fakeReq = {
+      const fakeReq: TestRequest = {
         brute: {
           reset: sinon.stub().callsArg(0),
         },
@@ -140,7 +156,7 @@ describe('Session controller', function () {
             password: 'qu33nRul35',
           },
         })
-        .then((fn) => {
+        .then((fn: SessionMiddleware) => {
           fn(fakeReq, fakeRes, fakeNext);
         })
         .then(function () {
@@ -157,7 +173,7 @@ describe('Session controller', function () {
     });
 
     it('it creates a non-verified session when the user has logged in before', function () {
-      const fakeReq = {
+      const fakeReq: TestRequest = {
         brute: {
           reset: sinon.stub().callsArg(0),
         },
@@ -177,7 +193,7 @@ describe('Session controller', function () {
             password: 'qu33nRul35',
           },
         })
-        .then((fn) => {
+        .then((fn: SessionMiddleware) => {
           fn(fakeReq, fakeRes, fakeNext);
         })
         .then(function () {
@@ -203,7 +219,7 @@ describe('Session controller', function () {
 
       return sessionController
         .delete()
-        .then((fn) => {
+        .then((fn: SessionMiddleware) => {
           fn(fakeReq, fakeRes, fakeNext);
         })
         .then(function () {

From 2057fe847d1f743ad3543cacd7061bac82cff7fa Mon Sep 17 00:00:00 2001
From: Evan Hahn 
Date: Mon, 28 Sep 2026 21:09:31 +0200
Subject: [PATCH 12/21] Fixed flaky "queue request" middleware test (#31049)

no ref

This is a test-only cleanup.
---
 .../parent/middleware/queue-request.test.ts   | 52 +++++++++----------
 1 file changed, 26 insertions(+), 26 deletions(-)

diff --git a/ghost/core/test/unit/server/web/parent/middleware/queue-request.test.ts b/ghost/core/test/unit/server/web/parent/middleware/queue-request.test.ts
index 31d14fa45de..af9e8ffbf7d 100644
--- a/ghost/core/test/unit/server/web/parent/middleware/queue-request.test.ts
+++ b/ghost/core/test/unit/server/web/parent/middleware/queue-request.test.ts
@@ -1,5 +1,5 @@
 import assert from 'node:assert/strict';
-import { EventEmitter } from 'node:events';
+import { EventEmitter, once } from 'node:events';
 import http from 'node:http';
 import type { AddressInfo } from 'node:net';
 import express from 'express';
@@ -30,9 +30,7 @@ describe('Queue request middleware', function () {
     });
 
     server = app.listen(0, '127.0.0.1');
-    await new Promise((resolve) => {
-      server.once('listening', resolve);
-    });
+    await once(server, 'listening');
     port = (server.address() as AddressInfo).port;
   }
 
@@ -45,6 +43,16 @@ describe('Queue request middleware', function () {
   }
 
   function get(path: string) {
+    const serverReceivedRequest = new Promise((resolve) => {
+      const onRequest = (request: http.IncomingMessage, res: http.ServerResponse) => {
+        if (request.url === path) {
+          server.off('request', onRequest);
+          resolve(res);
+        }
+      };
+      server.on('request', onRequest);
+    });
+
     // URL string: host/port options throw Invalid URL once another file has loaded Sentry's http wrapper
     const req = http.get(`http://127.0.0.1:${port}${path}`, { agent: false });
     const response = new Promise<{ status?: number; body: string }>((resolve, reject) => {
@@ -61,7 +69,7 @@ describe('Queue request middleware', function () {
     // callers that abort the request don't await the response
     response.catch(() => {});
 
-    return { req, response };
+    return { req, serverReceivedRequest, response };
   }
 
   beforeEach(function () {
@@ -144,15 +152,12 @@ describe('Queue request middleware', function () {
   it('limits concurrency and starts queued requests in arrival order', async function () {
     await listen(2);
     get('/hold/1');
+    await heldCount(1);
     get('/hold/2');
     await heldCount(2);
 
-    get('/hold/3');
-    get('/hold/4');
-    // give the queued requests time to arrive
-    await new Promise((resolve) => {
-      setTimeout(resolve, 50);
-    });
+    await get('/hold/3').serverReceivedRequest;
+    await get('/hold/4').serverReceivedRequest;
     assert.equal(held.length, 2);
     assert.equal(held[0].req.queueDepth, 0);
 
@@ -175,6 +180,7 @@ describe('Queue request middleware', function () {
     await heldCount(1);
 
     const queued = get('/instant');
+    await queued.serverReceivedRequest;
     await new Promise((resolve) => {
       setTimeout(resolve, 50);
     });
@@ -193,17 +199,12 @@ describe('Queue request middleware', function () {
     await heldCount(1);
 
     const abandoned = get('/hold/abandoned');
-    await new Promise((resolve) => {
-      setTimeout(resolve, 50);
-    });
+    const abandonedRes = await abandoned.serverReceivedRequest;
     const next = get('/instant');
-    await new Promise((resolve) => {
-      setTimeout(resolve, 50);
-    });
+    await next.serverReceivedRequest;
+    const abandonedClosed = once(abandonedRes, 'close');
     abandoned.req.destroy();
-    await new Promise((resolve) => {
-      setTimeout(resolve, 50);
-    });
+    await abandonedClosed;
 
     held[0].res.end();
     const { status, body } = await next.response;
@@ -233,17 +234,16 @@ describe('Queue request middleware', function () {
     const first = get('/hold/1');
     await heldCount(1);
 
-    get('/hold/2');
-    get('/hold/3');
-    await new Promise((resolve) => {
-      setTimeout(resolve, 50);
-    });
+    await get('/hold/2').serverReceivedRequest;
+    await get('/hold/3').serverReceivedRequest;
+    const firstClosed = once(held[0].res, 'close');
 
     held[0].res.end('done');
     await first.response;
+    await firstClosed;
     await heldCount(2);
     await new Promise((resolve) => {
-      setTimeout(resolve, 50);
+      setImmediate(resolve);
     });
 
     // a double release would have started request 3 alongside request 2

From 29345c93b909fca8f38676698fe1abf51b71cb49 Mon Sep 17 00:00:00 2001
From: Evan Hahn 
Date: Mon, 28 Sep 2026 21:17:35 +0200
Subject: [PATCH 13/21] Added tests for frontend add/edit automated email APIs
 (#31048)

towards https://linear.app/ghost/issue/NY-1617

This is a test-only change.

I think this useful on its own, but it'll also make an upcoming change
easier.
---
 .../test/unit/api/automated-emails.test.ts    | 58 +++++++++++++++++++
 1 file changed, 58 insertions(+)
 create mode 100644 apps/admin-x-framework/test/unit/api/automated-emails.test.ts

diff --git a/apps/admin-x-framework/test/unit/api/automated-emails.test.ts b/apps/admin-x-framework/test/unit/api/automated-emails.test.ts
new file mode 100644
index 00000000000..2244af2a326
--- /dev/null
+++ b/apps/admin-x-framework/test/unit/api/automated-emails.test.ts
@@ -0,0 +1,58 @@
+import { act } from '@testing-library/react';
+import { renderHookWithProviders } from '../../../src/test/test-utils';
+import {
+  useAddAutomatedEmail,
+  useEditAutomatedEmail,
+  type AutomatedEmail,
+} from '../../../src/api/automated-emails';
+import { withMockFetch } from '../../utils/mock-fetch';
+
+const email: AutomatedEmail = {
+  id: 'email-id',
+  name: 'Free member welcome flow',
+  slug: 'member-welcome-email-free',
+  status: 'active',
+  subject: 'Welcome!',
+  lexical: null,
+  sender_name: null,
+  sender_email: null,
+  sender_reply_to: null,
+  created_at: '2026-01-01T00:00:00.000Z',
+  updated_at: null,
+};
+
+describe('automated email APIs', () => {
+  describe('useAddAutomatedEmail', () => {
+    it('sends the creation payload', async () => {
+      await withMockFetch({ json: { automated_emails: [email] } }, async (mock) => {
+        const { result } = renderHookWithProviders(() => useAddAutomatedEmail());
+        await act(async () => {
+          await result.current.mutateAsync(email);
+        });
+        const request = mock.calls.find(
+          ([, options]: [unknown, RequestInit]) => options.method === 'POST',
+        );
+        expect(request[0]).toBe('http://localhost:3000/ghost/api/admin/automated_emails/');
+        expect(JSON.parse(request[1].body)).toEqual({ automated_emails: [email] });
+      });
+    });
+  });
+
+  describe('useEditAutomatedEmail', () => {
+    it('sends the update payload', async () => {
+      await withMockFetch({ json: { automated_emails: [email] } }, async (mock) => {
+        const { result } = renderHookWithProviders(() => useEditAutomatedEmail());
+        await act(async () => {
+          await result.current.mutateAsync(email);
+        });
+        const request = mock.calls.find(
+          ([, options]: [unknown, RequestInit]) => options.method === 'PUT',
+        );
+        expect(request[0]).toBe('http://localhost:3000/ghost/api/admin/automated_emails/email-id/');
+        expect(JSON.parse(request[1].body)).toEqual({
+          automated_emails: [email],
+        });
+      });
+    });
+  });
+});

From f65963ad1e514ee03453e5708b426717f1698448 Mon Sep 17 00:00:00 2001
From: Chris Raible 
Date: Mon, 28 Sep 2026 12:25:23 -0700
Subject: [PATCH 14/21] Formatted tinybird api_kpis.pipe file (#30831)

no refs

[I recommend reviewing this with whitespace
hidden.](https://github.com/TryGhost/Ghost/pull/30831/changes?w=1)

This PR is code formatting only; there should be no user-facing changes.
I generated this PR by running `tb fmt` on the `api_kpis.pipe` and
committed the result unmodified. Formatting these files will make it
easier to read the diff for future changes.
---
 .../data/tinybird/endpoints/api_kpis.pipe     | 309 +++++++++++-------
 1 file changed, 182 insertions(+), 127 deletions(-)

diff --git a/ghost/core/core/server/data/tinybird/endpoints/api_kpis.pipe b/ghost/core/core/server/data/tinybird/endpoints/api_kpis.pipe
index c5f6b4b47a6..fa0ec1eef75 100644
--- a/ghost/core/core/server/data/tinybird/endpoints/api_kpis.pipe
+++ b/ghost/core/core/server/data/tinybird/endpoints/api_kpis.pipe
@@ -3,106 +3,122 @@ TOKEN "axis" READ
 
 NODE timeseries
 SQL >
-
     %
-        {% set _single_day = defined(date_from) and defined(date_to) and day_diff(date_from, date_to) == 0 %}
-        with
-            {% if defined(date_from) %}
-                toStartOfDay(
-                    toDate(
-                        {{
-                            Date(
-                                date_from,
-                                description="Starting day for filtering a date range",
-                                required=False,
-                            )
-                        }}
-                    )
-                ) as start,
-            {% else %} toStartOfDay(timestampAdd(today(), interval -7 day)) as start,
-            {% end %}
-            {% if defined(date_to) %}
-                toStartOfDay(
-                    toDate(
-                        {{
-                            Date(
-                                date_to,
-                                description="Finishing day for filtering a date range",
-                                required=False,
-                            )
-                        }}
-                    )
-                ) as end
-            {% else %} toStartOfDay(today()) as end
-            {% end %}
-            {% if _single_day %}
-                ,
-                {% if defined(current_time) %}
-                    toDateTime({{ String(current_time, description="Current time override for tests", required=False) }}, {{String(timezone, 'Etc/UTC', description="Site timezone", required=True)}})
-                {% else %}
-                    toTimezone(now(), {{String(timezone, 'Etc/UTC', description="Site timezone", required=True)}})
-                {% end %} as current_time,
-                if(
-                    toDate(end) = toDate(current_time),
-                    least(
-                        timestampAdd(end, interval 1 day),
-                        timestampAdd(start, toIntervalHour(toHour(current_time) + 1))
-                    ),
-                    timestampAdd(end, interval 1 day)
-                ) as end_exclusive
-            {% end %}
-        {% if _single_day %}
-            select
-                arrayJoin(
-                    arrayMap(
-                        x -> toDateTime(toString(toDateTime(x)), {{String(timezone, 'Etc/UTC', description="Site timezone", required=True)}}),
-                        range(
-                            toUInt32(toDateTime(start)), toUInt32(end_exclusive), 3600
+    {% set _single_day = (
+        defined(date_from) and defined(date_to) and day_diff(date_from, date_to) == 0
+    ) %}
+    with
+        {% if defined(date_from) %}
+            toStartOfDay(
+                toDate(
+                    {{
+                        Date(
+                            date_from,
+                            description="Starting day for filtering a date range",
+                            required=False,
                         )
+                    }}
+                )
+            ) as start,
+        {% else %} toStartOfDay(timestampAdd(today(), interval -7 day)) as start,
+        {% end %}
+        {% if defined(date_to) %}
+            toStartOfDay(
+                toDate(
+                    {{
+                        Date(
+                            date_to,
+                            description="Finishing day for filtering a date range",
+                            required=False,
+                        )
+                    }}
+                )
+            ) as
+    end {% else %} toStartOfDay(today()) as
+    end {% end %}
+    {% if _single_day %}
+        ,
+        {% if defined(current_time) %}
+            toDateTime(
+                {{
+                    String(
+                        current_time, description="Current time override for tests", required=False
                     )
-                ) as date
+                }}, {{ String(timezone, 'Etc/UTC', description="Site timezone", required=True) }}
+            )
         {% else %}
-            select
-                arrayJoin(
-                    arrayMap(
-                        x -> toDate(x),
-                        range(toUInt32(start), toUInt32(timestampAdd(end, interval 1 day)), 24 * 3600)
-                    )
-                ) as date
-        {% end %}
-
+            toTimezone(
+                now(), {{ String(timezone, 'Etc/UTC', description="Site timezone", required=True) }}
+            )
+        {% end %} as current_time,
+        if(toDate(end) = toDate(current_time),
+        least(timestampAdd(end, interval 1 day),
+        timestampAdd(start, toIntervalHour(toHour(current_time) + 1))
+    ),
+    timestampAdd(end,
+    interval 1 day
+    )
+    ) as end_exclusive
+    {% end %}
+    {% if _single_day %}
+        select
+            arrayJoin(
+                arrayMap(
+                    x -> toDateTime(
+                        toString(toDateTime(x)),
+                        {{ String(timezone, 'Etc/UTC', description="Site timezone", required=True) }}
+                    ),
+                    range(toUInt32(toDateTime(start)), toUInt32(end_exclusive), 3600)
+                )
+            ) as date
+    {% else %}
+        select
+            arrayJoin(
+                arrayMap(
+                    x -> toDate(x),
+                    range(toUInt32(start), toUInt32(timestampAdd(end, interval 1 day)),
+                    24 * 3600
+                )
+            )
+    ) as date
+    {% end %}
 
 NODE session_metrics
 DESCRIPTION >
     Calculate session-level metrics (visits, pageviews, bounce rate, avg session duration)
 
 SQL >
-
     %
-        select
-            site_uuid,
-            {% if defined(date_from) and defined(date_to) and day_diff(date_from, date_to) == 0 %}
-                toStartOfHour(toTimezone(first_pageview, {{String(timezone, 'Etc/UTC', description="Site timezone", required=True)}})) as date,
-            {% else %}
-                toDate(toTimezone(first_pageview, {{String(timezone, 'Etc/UTC', description="Site timezone", required=True)}})) as date,
-            {% end %}
-            session_id,
-            pageviews,
-            is_bounce,
-            duration as session_sec
-        from mv_session_data sd
-            inner join filtered_sessions fs
-                on fs.session_id = sd.session_id
-        where
-            site_uuid = {{ String(site_uuid, 'mock_site_uuid', description="Tenant ID", required=True) }}
-
+    select
+        site_uuid,
+        {% if defined(date_from) and defined(date_to) and day_diff(date_from, date_to) == 0 %}
+            toStartOfHour(
+                toTimezone(
+                    first_pageview,
+                    {{ String(timezone, 'Etc/UTC', description="Site timezone", required=True) }}
+                )
+            ) as date,
+        {% else %}
+            toDate(
+                toTimezone(
+                    first_pageview,
+                    {{ String(timezone, 'Etc/UTC', description="Site timezone", required=True) }}
+                )
+            ) as date,
+        {% end %}
+        session_id,
+        pageviews,
+        is_bounce,
+        duration as session_sec
+    from mv_session_data sd
+    inner join filtered_sessions fs on fs.session_id = sd.session_id
+    where site_uuid = {{ String(site_uuid, 'mock_site_uuid', description="Tenant ID", required=True) }}
 
 NODE data
 DESCRIPTION >
     Calculate KPIs per time period
 
 SQL >
-
     select
         a.date,
         uniq(distinct s.session_id) as visits,
@@ -114,59 +130,98 @@ SQL >
     group by a.date
     order by a.date
 
-
 NODE pathname_pageviews
 DESCRIPTION >
     Calculate pageviews for specific pathname with time granularity handling
 
 SQL >
-
     %
-            select
-                {% if defined(date_from) and defined(date_to) and day_diff(date_from, date_to) == 0 %}
-                    toStartOfHour(toTimezone(timestamp, {{String(timezone, 'Etc/UTC', description="Site timezone", required=True)}})) as date,
-                {% else %}
-                    toDate(toTimezone(timestamp, {{String(timezone, 'Etc/UTC', description="Site timezone", required=True)}})) as date,
-                {% end %}
-                count() pageviews
-            from timeseries a
-            inner join _mv_hits h on
-                {% if defined(date_from) and defined(date_to) and day_diff(date_from, date_to) == 0 %}
-                    a.date = toStartOfHour(toTimezone(timestamp, {{String(timezone, 'Etc/UTC', description="Site timezone", required=True)}}))
-                {% else %}
-                    a.date = toDate(toTimezone(timestamp, {{String(timezone, 'Etc/UTC', description="Site timezone", required=True)}}))
-                {% end %}
-            inner join filtered_sessions fs
-                on fs.session_id = h.session_id
-            where
-                site_uuid = {{ String(site_uuid, 'mock_site_uuid', description="Tenant ID", required=True) }}
-                {% if defined(member_status) %}
-                    and member_status IN (
-                        select arrayJoin(
-                            {{ Array(member_status, "'undefined', 'free', 'paid'", description="Member status to filter on", required=False) }}
-                            || if('paid' IN {{ Array(member_status) }}, ['comped', 'gift'], [])
-                        )
+    select
+        {% if defined(date_from) and defined(date_to) and day_diff(date_from, date_to) == 0 %}
+            toStartOfHour(
+                toTimezone(
+                    timestamp,
+                    {{ String(timezone, 'Etc/UTC', description="Site timezone", required=True) }}
+                )
+            ) as date,
+        {% else %}
+            toDate(
+                toTimezone(
+                    timestamp,
+                    {{ String(timezone, 'Etc/UTC', description="Site timezone", required=True) }}
+                )
+            ) as date,
+        {% end %}
+        count() pageviews
+    from timeseries a
+    inner join
+        _mv_hits h
+        on {% if defined(date_from) and defined(date_to) and day_diff(date_from, date_to) == 0 %}
+            a.date = toStartOfHour(
+                toTimezone(
+                    timestamp,
+                    {{ String(timezone, 'Etc/UTC', description="Site timezone", required=True) }}
+                )
+            )
+        {% else %}
+            a.date = toDate(
+                toTimezone(
+                    timestamp,
+                    {{ String(timezone, 'Etc/UTC', description="Site timezone", required=True) }}
+                )
+            )
+        {% end %}
+    inner join filtered_sessions fs on fs.session_id = h.session_id
+    where
+        site_uuid = {{ String(site_uuid, 'mock_site_uuid', description="Tenant ID", required=True) }}
+        {% if defined(member_status) %}
+            and member_status IN (
+                select
+                    arrayJoin(
+                        {{
+                            Array(
+                                member_status,
+                                "'undefined', 'free', 'paid'",
+                                description="Member status to filter on",
+                                required=False,
+                            )
+                        }} || if('paid' IN {{ Array(member_status) }}, ['comped', 'gift'], [])
                     )
-                {% end %}
-                {% if defined(location) %} and location = {{ String(location, description="Location to filter on", required=False) }} {% end %}
-                {% if defined(pathname) %} and pathname = {{ String(pathname, description="Pathname to filter on", required=False) }} {% end %}
-                {% if defined(post_uuid) %} and post_uuid = {{String(post_uuid, description="Post UUID to filter on", required=False) }} {% end %}
-                {% if defined(gift_link) %}{% if gift_link == 'false' or gift_link == '0' %} and gift_link = '' {% else %} and gift_link != '' {% end %}{% end %}
-            group by date
-            order by date
-
+            )
+        {% end %}
+        {% if defined(location) %}
+            and location = {{ String(location, description="Location to filter on", required=False) }}
+        {% end %}
+        {% if defined(pathname) %}
+            and pathname = {{ String(pathname, description="Pathname to filter on", required=False) }}
+        {% end %}
+        {% if defined(post_uuid) %}
+            and post_uuid
+            = {{ String(post_uuid, description="Post UUID to filter on", required=False) }}
+        {% end %}
+        {% if defined(gift_link) %}
+            {% if gift_link == 'false' or gift_link == '0' %} and gift_link = ''
+            {% else %} and gift_link != ''
+            {% end %}
+        {% end %}
+    group by date
+    order by date
 
 NODE finished_data
 SQL >
-
     %
-            select
-                a.date as date,
-                coalesce(b.visits, 0) as visits,
-                {% if defined(pathname) or defined(post_uuid) or defined(gift_link) %}coalesce(c.pageviews, 0){% else %}coalesce(b.pageviews, 0){% end %} as pageviews,
-                coalesce(b.bounce_rate, 0) as bounce_rate,
-                coalesce(b.avg_session_sec, 0) as avg_session_sec
-            from timeseries a
-            left join data b on a.date = b.date
-            {% if defined(pathname) or defined(post_uuid) or defined(gift_link) %}left join pathname_pageviews c on a.date = c.date{% end %}
+    select
+        a.date as date,
+        coalesce(b.visits, 0) as visits,
+        {% if defined(pathname) or defined(post_uuid) or defined(gift_link) %} coalesce(c.pageviews, 0)
+        {% else %} coalesce(b.pageviews, 0)
+        {% end %} as pageviews,
+        coalesce(b.bounce_rate, 0) as bounce_rate,
+        coalesce(b.avg_session_sec, 0) as avg_session_sec
+    from timeseries a
+    left join data b on a.date = b.date
+    {% if defined(pathname) or defined(post_uuid) or defined(gift_link) %}
+        left join pathname_pageviews c on a.date = c.date
+    {% end %}
+
 TYPE ENDPOINT

From f6e5f78b55d677fc2a6c8d5e0722824064d5dbbb Mon Sep 17 00:00:00 2001
From: Steve Larson <9larsons@gmail.com>
Date: Mon, 28 Sep 2026 21:43:05 +0200
Subject: [PATCH 15/21] Fixed welcome email rows ignoring Edit clicks while
 emails load (#31046)

no ref

Welcome email rows render before the automated emails list loads, but
the edit handler ignores clicks until it does, so early clicks were
silently dropped. This also flaked the welcome-email acceptance specs on
slow CI. The row's edit triggers are now disabled until the list has
loaded.
---
 apps/admin/src/settings/membership/member-emails.tsx | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/apps/admin/src/settings/membership/member-emails.tsx b/apps/admin/src/settings/membership/member-emails.tsx
index 6fb6dc1301e..9e702cc8e0a 100644
--- a/apps/admin/src/settings/membership/member-emails.tsx
+++ b/apps/admin/src/settings/membership/member-emails.tsx
@@ -64,6 +64,7 @@ const EmailPreviewRow: React.FC<{