🐛 Fixed periodic host limits in Admin - #29837
Conversation
no ref Admin's limit service loaded limits without the `subscription` argument that @tryghost/limit-service requires for periodic limits, so any configured maxPeriodic limit threw an IncorrectUsageError in the browser. Loading stops at the first limit that can't be built, so a site with an emails limit ended up with no limits registered at all, and the rejection surfaced in the editor's publish flow. - passes the subscription through, resolved from hostSettings the same way the server does - adds a client-side count query for emails, since the default one counts recipients via knex - skips the emails check for authors and contributors, who can't browse emails, matching how the members limit is already handled - tolerates a limit that can't be built instead of leaving Admin with none
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe limits service now registers subscription-aware monthly limits and filters periodic limits when subscription data is unavailable. Email limits use a browser-compatible query that sums recipients created after the period start. Publishing skips checks for authors and contributors and handles an absent limiter. Email verification remains independently evaluated. Tests cover subscription handling, count aggregation, filtering, empty results, Sinon cleanup, and the account-review hold. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/ember-admin/app/services/limit.js`:
- Around line 79-86: Update loadLimits and its Object.keys(limits).forEach
registration flow so an IncorrectUsageError from a missing subscription for a
<name>.maxPeriodic limit is handled during iteration, allowing later valid
limits to register. Preserve propagation of other errors and add or update
coverage with an invalid periodic limit followed by a valid non-periodic limit.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 18be5678-060a-46e6-b66e-08c90c0caf10
📒 Files selected for processing (3)
apps/ember-admin/app/services/limit.jsapps/ember-admin/app/utils/publish-options.jsapps/ember-admin/tests/unit/services/limit-test.js
no ref loadLimits() registers limits in a forEach, so a periodic limit that can't be built stops the loop and drops every limit declared after it. Catching the error afterwards kept Admin running but still lost those later limits. Filters periodic limits out when there is no subscription to build them from, so the rest register as normal.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/ember-admin/app/services/limit.js`:
- Around line 69-71: Update the subscription construction in the limit-loading
flow before usableLimits() is called so hostSettings.subscription is only
created when its start value exists. Ensure missing subscription.start produces
no subscription object, allowing usableLimits() to filter out periodic limits
and preventing getEmailsCount() from receiving an undefined startDate.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0aa595e1-dd1d-44f6-b34e-cea460432ffe
📒 Files selected for processing (2)
apps/ember-admin/app/services/limit.jsapps/ember-admin/tests/unit/services/limit-test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/ember-admin/tests/unit/services/limit-test.js
no ref A hostSettings.subscription with no start built a subscription object anyway, which passed the periodic-limit filter and then threw on the missing start date. The emails count query would have received an undefined start too.
9larsons
left a comment
9larsons
left a comment
There was a problem hiding this comment.
There’s one performance concern in the new browser-side email count query.
no ref The emails browse response carries the full html, plaintext and source of every record, and the query is awaited while the publish options load. Selecting id and email_count keeps the count and drops the bodies.
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx run @tryghost/admin:test:acceptance |
✅ Succeeded | 8m | View ↗ |
nx run-many -t test:unit -p @tryghost/admin |
✅ Succeeded | 4m 3s | View ↗ |
nx run ghost-admin:test |
✅ Succeeded | 3m 4s | View ↗ |
nx run @tryghost/admin:build |
✅ Succeeded | 1m 52s | View ↗ |
nx run ghost-monorepo:lint:boundaries |
✅ Succeeded | 22s | View ↗ |
nx run-many -t lint -p ghost-admin,@tryghost/ad... |
✅ Succeeded | 1m 25s | View ↗ |
nx run-many --target=build --projects=tag:publi... |
✅ Succeeded | 1s | View ↗ |
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗
☁️ Nx Cloud last updated this comment at 2026-08-10 22:36:02 UTC
9larsons
left a comment
9larsons
left a comment
There was a problem hiding this comment.
One additional interaction surfaced in a second pass.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #29837 +/- ##
==========================================
+ Coverage 75.40% 75.45% +0.04%
==========================================
Files 1606 1606
Lines 140381 140397 +16
Branches 17389 17406 +17
==========================================
+ Hits 105860 105931 +71
+ Misses 33444 33418 -26
+ Partials 1077 1048 -29
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
no ref With periodic email limits now registering, an admin or editor under their limit took the first branch and skipped the verification check, so the publish UI offered email during an account-review hold and the rejection only came back from the server. The server applies both in this order; this now mirrors it, keeping the limit error ahead of the hold.
Head branch was pushed to by a user without write access
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/ember-admin/tests/acceptance/editor/publish-flow-test.js`:
- Around line 694-709: Update the publish-flow test setup around the `/emails/`
handler and its warning assertion to capture or inspect the request made by
`LimitsService.getEmailsCount`. Assert that the email-count query reaches the
endpoint with the expected filter, fields, and limit parameters before asserting
the warning, ensuring the emails limiter path is exercised.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 62844348-1e18-4f9b-85e7-d03c9949ce79
📒 Files selected for processing (2)
apps/ember-admin/app/utils/publish-options.jsapps/ember-admin/tests/acceptance/editor/publish-flow-test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/ember-admin/app/utils/publish-options.js
no ref The test returned an empty emails response but never checked the request was made, so it would have passed on the verification hold alone with the emails limiter never registering.
2 participants
Admin's limit service loaded limits without the
subscriptionargument that @tryghost/limit-service requires for periodic limits, so any configured maxPeriodic limit threw an IncorrectUsageError in the browser. Loading stops at the first limit that can't be built, so a site with an emails limit ended up with no limits registered at all, and the rejection surfaced in the editor's publish flow.Got some code for us? Awesome 🎊!
Please take a minute to explain the change you're making:
Please check your PR against these items:
We appreciate your contribution! 🙏