-
-
Notifications
You must be signed in to change notification settings - Fork 11.3k
🐛 Fixed sorting in Growth tab Sources table #25822
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
🐛 Fixed sorting in Growth tab Sources table #25822
Conversation
Co-authored-by: troy <troy@ghost.org>
|
Cursor Agent can help with this pull request. Just |
WalkthroughRenames the sorting option used by the referrers stats service from Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @ghost/core/core/server/services/stats/referrers-stats-service.js:
- Around line 369-370: Tests still pass the old parameter name; update calls to
use the new "order" key so getTopSourcesWithRange(options) receives it
correctly: locate the test calls that currently pass {orderBy: 'signups desc'},
{orderBy: 'source desc'}, and {orderBy: 'signups desc', limit: 3} and rename
those keys to {order: 'signups desc'}, {order: 'source desc'}, and {order:
'signups desc', limit: 3} respectively so they match the function
getTopSourcesWithRange's destructured parameter (order: orderBy).
🧹 Nitpick comments (1)
ghost/core/core/server/services/stats/referrers-stats-service.js (1)
418-447: Optional: Consider documenting or supporting ascending sort order.The sorting logic at Line 446 always sorts in descending order (
valueA < valueB ? 1 : valueA > valueB ? -1 : 0) regardless of the direction specified in theorderparameter string (e.g., "signups asc" would still sort descending). While the JSDoc examples only show descending sorts and Line 418's comment mentions "only allow descending sorts for sources," this could be confusing if a caller passes an ascending order.Consider either:
- Updating the JSDoc to explicitly state that only descending order is supported
- Or implementing ascending sort support by parsing and using the direction from the
orderBystring📝 Option 1: Document descending-only behavior
- * @param {string} [options.order='signups desc'] - Sort order: 'signups desc', 'paid_conversions desc', 'mrr desc', 'source desc' + * @param {string} [options.order='signups desc'] - Sort order (descending only): 'signups desc', 'paid_conversions desc', 'mrr desc', 'source desc'
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ghost/core/core/server/services/stats/referrers-stats-service.jsghost/core/core/server/services/stats/stats-service.js
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-08T10:26:38.700Z
Learnt from: rob-ghost
Repo: TryGhost/Ghost PR: 25791
File: ghost/core/core/server/api/endpoints/member-comment-ban.js:64-68
Timestamp: 2026-01-08T10:26:38.700Z
Learning: In the Ghost API, endpoints rely on the serialization layer to prepare frame.data[docName] as a non-empty array before query() executes. Endpoints access frame.data[docName][0] directly (e.g., frame.data.comment_bans[0], frame.data.members[0], frame.data.posts[0]) without per-endpoint validation. This pattern is common across API endpoints. When maintaining or creating endpoints, avoid duplicating validation for frame.data[docName] and ensure the serializer guarantees the shape and non-emptiness. If you add a new endpoint that uses this frame.data[docName], follow the same assumption and avoid redundant checks unless there's a documented exception.
Applied to files:
ghost/core/core/server/services/stats/referrers-stats-service.jsghost/core/core/server/services/stats/stats-service.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: [Optional] E2E Tests (React 4/4)
- GitHub Check: E2E Tests (Ember 4/4)
- GitHub Check: [Optional] E2E Tests (React 3/4)
- GitHub Check: [Optional] E2E Tests (React 2/4)
- GitHub Check: E2E Tests (Ember 1/4)
- GitHub Check: E2E Tests (Ember 2/4)
- GitHub Check: E2E Tests (Ember 3/4)
- GitHub Check: [Optional] E2E Tests (React 1/4)
- GitHub Check: Acceptance tests (Node 22.18.0, sqlite3)
- GitHub Check: Legacy tests (Node 22.18.0, mysql8)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Unit tests (Node 22.18.0)
- GitHub Check: Acceptance tests (Node 22.18.0, mysql8)
🔇 Additional comments (2)
ghost/core/core/server/services/stats/stats-service.js (1)
235-245: JSDoc parameter name correctly updated.The JSDoc update from
options.orderBytooptions.orderaligns with the actual parameter name sent by the API and the destructuring change inreferrers-stats-service.js. The default value and allowed sort options are preserved.ghost/core/core/server/services/stats/referrers-stats-service.js (1)
358-368: JSDoc correctly updated to reflect the new parameter name.The parameter documentation change from
options.orderBytooptions.orderaligns with the destructuring change below and matches the parameter name sent by the API.
ghost/core/core/server/services/stats/referrers-stats-service.js
Outdated
Show resolved
Hide resolved
cmraible
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one, works for me 👌
closes https://linear.app/ghost/issue/NY-696
order, but the backend service was expectingorderBy. This meant the sort parameter was never correctly applied, and data always appeared in the default orderghost/core/core/server/services/stats/referrers-stats-service.jsto correctly map the incomingorderparameter to the internalorderByvariable.