Added new top referrers endpoint for post analytics #23149
Conversation
|
""" WalkthroughThis change set introduces a new feature for tracking and displaying post-level referrer statistics throughout the application stack. On the backend, a new API endpoint Possibly related PRs
Suggested labels
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
apps/posts/src/hooks/usePostReferrers.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-react-hooks". (The package "eslint-plugin-react-hooks" was not found when loaded as a Node module from the directory "/apps/posts".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-react-hooks" was referenced from the config file in "apps/posts/.eslintrc.cjs". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (11)
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
cmraible
force-pushed
the
chris-prod-1542-adjust-conversion-attribution-to-reflect-free-members-paid
branch
from
bbf9196 to
a58a993
Compare
cmraible
force-pushed
the
chris-prod-1542-adjust-conversion-attribution-to-reflect-free-members-paid
branch
from
9e11d5f to
eaa4211
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
apps/posts/src/views/PostAnalytics/Growth.tsx (1)
27-36: 🛠️ Refactor suggestionPotential undefined access – guard
totals
totalsis used unconditionally onceisLoadingis false. An API error or empty payload yieldsundefined, causing runtime crashes. Provide a default object or nullish coalescing.-<KpiCardValue>{formatNumber(totals.free_members)}</KpiCardValue> +<KpiCardValue>{formatNumber(totals?.free_members ?? 0)}</KpiCardValue>
🧹 Nitpick comments (4)
apps/posts/src/views/PostAnalytics/Growth.tsx (1)
10-12: Empty interface – switch to a type alias or remove altogether
postAnalyticsPropsis declared but contains no members, triggering Biome’snoEmptyInterfacerule and adding dead weight to the bundle.-interface postAnalyticsProps {} +type PostAnalyticsProps = Record<string, never>; // or remove entirely if unusedghost/core/core/server/services/stats/PostsStatsService.js (1)
316-344: Date parsing relies onnew Date()– prone to TZ and format drift
new Date('YYYY-MM-DD')is implementation-dependent (UTC vs local). A safer approach is to:
- Use
Date.parseonly with ISO strings containing timezone, or- Validate with a library (Luxon / Day.js) and convert to UTC explicitly.
-const fromDate = new Date(options.date_from); +const {DateTime} = require('luxon'); +const fromDate = DateTime.fromISO(options.date_from, {zone: 'utc'}); +if (fromDate.isValid) { + query.where(dateColumn, '>=', fromDate.toSQLDate()); +}This avoids silent off-by-one-day errors.
ghost/core/test/unit/server/services/stats/posts.test.js (2)
121-138: Add indexes onreferrer_sourcein test schema for realistic query plansReal queries aggregate by
referrer_source; without an index, the tests may pass but mask performance issues in production. A small change keeps the schema aligned:- table.string('referrer_source'); + table.string('referrer_source').index();Same applies to
members_subscription_created_events.
287-289: Variable names (member_future) are misleading
member_futureis timestamped 30 days ago, older thanmember_past(10 days ago).
Consider renaming to convey temporal intent (e.g.member_older) to avoid confusion for future maintainers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/admin-x-framework/src/api/stats.ts(3 hunks)apps/posts/src/hooks/usePostReferrers.ts(1 hunks)apps/posts/src/views/PostAnalytics/Growth.tsx(3 hunks)ghost/core/core/server/api/endpoints/stats.js(1 hunks)ghost/core/core/server/services/stats/PostsStatsService.js(5 hunks)ghost/core/core/server/services/stats/StatsService.js(1 hunks)ghost/core/core/server/web/api/endpoints/admin/routes.js(2 hunks)ghost/core/test/unit/server/services/stats/posts.test.js(11 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
apps/admin-x-framework/src/api/stats.ts (1)
apps/posts/src/hooks/usePostReferrers.ts (1)
usePostReferrers(24-39)
apps/posts/src/hooks/usePostReferrers.ts (1)
apps/admin-x-framework/src/api/stats.ts (2)
PostReferrerStatItem(51-56)usePostReferrers(84-87)
🪛 Biome (1.9.4)
apps/posts/src/views/PostAnalytics/Growth.tsx
[error] 9-10: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Database tests (Node 20.11.1, sqlite3)
- GitHub Check: Database tests (Node 22.13.1, mysql8)
- GitHub Check: Regression tests (Node 20.11.1, mysql8)
- GitHub Check: Unit tests (Node 22.13.1)
- GitHub Check: Database tests (Node 20.11.1, mysql8)
- GitHub Check: Unit tests (Node 20.11.1)
- GitHub Check: Admin-X Settings tests
🔇 Additional comments (10)
ghost/core/core/server/web/api/endpoints/admin/routes.js (2)
6-6: Good addition of the labs import.The labs import is correctly added to enable feature flagging for the new traffic analytics routes.
158-162: Well-structured feature flag implementation for new traffic analytics routes.The new routes for advanced analytics features are properly guarded behind the
trafficAnalyticsfeature flag, which allows for controlled rollout. The new endpoint/stats/referrers/posts/:id/alphafollows the same authentication pattern as other similar endpoints.The "alpha" suffix in the route path clearly indicates this is an experimental feature, which is a good practice for API versioning.
ghost/core/core/server/services/stats/StatsService.js (1)
66-72: New method for retrieving post referrer statistics.The implementation follows the service's existing patterns - delegating to a specialized service (
posts) and returning the result. This is consistent with other methods likegetTopPosts.Note that unlike
getPostReferrers()at line 59, this new method returns the raw result instead of wrapping it in a{data, meta}structure. This slight inconsistency appears intentional based on how the data is used downstream.apps/posts/src/hooks/usePostReferrers.ts (3)
1-3: Appropriate imports for the React hook.Good practice to separate the API hook import (renamed to avoid confusion) and the React hooks needed for memoization.
4-22: Well-implemented helper function for calculating totals.The
calculateTotalsfunction:
- Handles the edge case of empty data arrays properly
- Safely handles possible null/undefined values using the
|| 0fallback- Returns a consistently structured object whether data exists or not
This makes the hook more robust against API inconsistencies or empty states.
24-39: Clean implementation of the custom hook with proper memoization.The hook follows React best practices:
- Uses
useMemoto avoid unnecessary recalculations of derived data- Properly handles the case when API data is not yet available with the
|| []fallback- Returns a clean, well-structured object with loading state and both raw and processed data
This implementation makes it easy for components to consume the hook and handle various states (loading, error, empty data).
apps/admin-x-framework/src/api/stats.ts (3)
51-61: Well-structured type definitions for post referrer statistics.The types are clearly defined with descriptive field names:
PostReferrerStatItemcaptures the essential metrics per referrer sourcePostReferrersResponseTypefollows the established pattern for API responses with stats and metadataThese types will provide good TypeScript support for components consuming this data.
68-68: Consistent data type constant declaration.The constant follows the naming pattern used for other data types in this file.
84-87: API hook implementation follows established patterns.The
usePostReferrershook:
- Uses the
createQueryWithIdfactory function consistent with other ID-based queries- Properly maps the ID parameter to the correct API path
- Points to the alpha version of the endpoint, matching the route registration
This maintains consistency with the project's API interaction patterns.
ghost/core/test/unit/server/services/stats/posts.test.js (1)
46-56: Column name hard-codes new behaviour – consider test-only guardThe helper now unconditionally writes
referrer_source. In production code this column is behind a feature flag; if the flag is toggled off, migrations may be absent and these tests will fail.
Add a capability check (e.g.if (await db.schema.hasColumn('members_created_events', 'referrer_source'))) or ensure the test suite always migrates the latest schema.
| {postReferrers?.map(row => ( | ||
| <TableRow key={row.source}> | ||
| <TableCell> | ||
| <a className='inline-flex items-center gap-2 font-medium' href={`https://${source.title}`} rel="noreferrer" target='_blank'> | ||
| <a className='inline-flex items-center gap-2 font-medium' href={`https://${row.source}`} rel="noreferrer" target='_blank'> | ||
| <img | ||
| className="size-4" | ||
| src={`https://www.faviconextractor.com/favicon/${source.title || 'direct'}?larger=true`} | ||
| src={`https://www.faviconextractor.com/favicon/${row.source || 'direct'}?larger=true`} | ||
| onError={(e: React.SyntheticEvent<HTMLImageElement>) => { | ||
| e.currentTarget.src = STATS_DEFAULT_SOURCE_ICON_URL; | ||
| }} /> | ||
| <span>{source.title || 'Direct'}</span> | ||
| <span>{row.source || 'Direct'}</span> | ||
| </a> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Invalid URL & favicon fetch when row.source is falsey
href={https://${row.source}} and the favicon call both misbehave on an empty value (e.g., “Direct” traffic is typically stored as null). Either:
- Filter out null sources in the query (backend), or
- Handle them gracefully in the UI:
-<a ... href={`https://${row.source}`} ...>
+<a
+ ...
+ href={row.source ? `https://${row.source}` : undefined}
+ rel="noopener noreferrer"
+ target={row.source ? '_blank' : undefined}
+>Also add noopener to the rel attribute for security.
| postReferrersAlpha: { | ||
| headers: { | ||
| cacheInvalidate: false | ||
| }, | ||
| options: [ | ||
| 'order', | ||
| 'limit', | ||
| 'date_from', | ||
| 'date_to', | ||
| 'timezone' | ||
| ], | ||
| data: [ | ||
| 'id' | ||
| ], | ||
| permissions: { | ||
| docName: 'posts', | ||
| method: 'browse' | ||
| }, | ||
| cache: statsService.cache, | ||
| generateCacheKeyData(frame) { | ||
| return { | ||
| method: 'getReferrersForPost', | ||
| data: { | ||
| id: frame.data.id | ||
| } | ||
| }; | ||
| }, | ||
| async query(frame) { | ||
| return await statsService.api.getReferrersForPost(frame.data.id, frame.options); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Missing payload validation for id, limit, and order
postReferrersAlpha directly forwards frame.data.id and frame.options to the service layer without confirming:
idexists and is a valid UUIDlimitis a positive integer within sane boundsordermatches the whitelist (free_members,paid_members,mrr)
An invalid request reaches the DB layer before being rejected, polluting logs and wasting CPU cycles.
Consider adding validation: { ... } or an explicit check in query() similar to other controllers.
🏁 Script executed:
#!/usr/bin/env bash
set -e
# Locate the stats endpoints file
echo "Locating stats.js..."
fd stats.js
# Inspect postReferrersAlpha definition
echo "Inspecting postReferrersAlpha snippet..."
rg -n "postReferrersAlpha" -A5 -B5 ghost/core/core/server/api/endpoints/stats.js
# Check for any existing validation in stats.js
echo "Searching for 'validation:' in stats.js..."
rg -n "validation:" ghost/core/core/server/api/endpoints/stats.js || echo "No validation property found in stats.js"
# Check how other endpoints declare validation
echo "Searching for 'validation:' across all endpoints..."
rg -n "validation:" ghost/core/core/server/api/endpointsLength of output: 10778
Add input validation to postReferrersAlpha endpoint
The postReferrersAlpha handler in ghost/core/core/server/api/endpoints/stats.js (lines 171–200) currently forwards frame.data.id and frame.options straight into the service layer. We should validate:
data.idexists and is a valid UUIDoptions.limitis an integer (e.g. ≥ 1, ≤ 100)options.orderis one of the allowed values (free_members,paid_members,mrr)
This prevents malformed requests hitting the DB and reduces error noise.
Suggested diff (insert before cache: statsService.cache,):
postReferrersAlpha: {
headers: {
cacheInvalidate: false
},
- options: [
+ options: [
'order',
'limit',
'date_from',
'date_to',
'timezone'
],
data: [
'id'
],
+ validation: {
+ data: {
+ id: { required: true, type: 'uuid' }
+ },
+ options: {
+ limit: { required: false, type: 'integer', min: 1, max: 100 },
+ order: { required: false, type: 'string', values: ['free_members', 'paid_members', 'mrr'] }
+ }
+ },
permissions: {
docName: 'posts',
method: 'browse'
},
cache: statsService.cache,
…
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| postReferrersAlpha: { | |
| headers: { | |
| cacheInvalidate: false | |
| }, | |
| options: [ | |
| 'order', | |
| 'limit', | |
| 'date_from', | |
| 'date_to', | |
| 'timezone' | |
| ], | |
| data: [ | |
| 'id' | |
| ], | |
| permissions: { | |
| docName: 'posts', | |
| method: 'browse' | |
| }, | |
| cache: statsService.cache, | |
| generateCacheKeyData(frame) { | |
| return { | |
| method: 'getReferrersForPost', | |
| data: { | |
| id: frame.data.id | |
| } | |
| }; | |
| }, | |
| async query(frame) { | |
| return await statsService.api.getReferrersForPost(frame.data.id, frame.options); | |
| } | |
| postReferrersAlpha: { | |
| headers: { | |
| cacheInvalidate: false | |
| }, | |
| options: [ | |
| 'order', | |
| 'limit', | |
| 'date_from', | |
| 'date_to', | |
| 'timezone' | |
| ], | |
| data: [ | |
| 'id' | |
| ], | |
| validation: { | |
| data: { | |
| id: { required: true, type: 'uuid' } | |
| }, | |
| options: { | |
| limit: { required: false, type: 'integer', min: 1, max: 100 }, | |
| order: { required: false, type: 'string', values: ['free_members', 'paid_members', 'mrr'] } | |
| } | |
| }, | |
| permissions: { | |
| docName: 'posts', | |
| method: 'browse' | |
| }, | |
| cache: statsService.cache, | |
| generateCacheKeyData(frame) { | |
| return { | |
| method: 'getReferrersForPost', | |
| data: { | |
| id: frame.data.id | |
| } | |
| }; | |
| }, | |
| async query(frame) { | |
| return await statsService.api.getReferrersForPost(frame.data.id, frame.options); | |
| } | |
| } |
| const results = await query | ||
| .orderBy(orderField, orderDirection) | ||
| .limit(limit); | ||
|
|
There was a problem hiding this comment.
WHERE NOT NULL drops ‘Direct’ traffic inadvertently
The final query excludes rows where referrer_source is NULL, yet the UI expects a “Direct” bucket. Either:
- Keep the rows and map
NULL → 'direct':
- .whereNotNull('ar.source');
+ // Keep NULL rows; label later- Or coalesce in-query:
-select 'ar.source',
+select coalesce(ar.source, 'direct') as source,Removing the filter restores parity between API and frontend.
Committable suggestion skipped: line range outside the PR's diff.
| _buildMrrReferrersSubquery(postId, options) { | ||
| const knex = this.knex; | ||
| let subquery = knex('members_subscription_created_events as msce') | ||
| .select('msce.referrer_source as source') | ||
| .sum('mpse.mrr_delta as mrr') | ||
| .join('members_paid_subscription_events as mpse', function () { | ||
| this.on('mpse.subscription_id', '=', 'msce.subscription_id'); | ||
| // Ensure we join on member_id as well for accuracy if subscription_id isn't unique across members? (Safeguard) | ||
| this.andOn('mpse.member_id', '=', 'msce.member_id'); | ||
| }) | ||
| .where('msce.attribution_id', postId) | ||
| .where('msce.attribution_type', 'post') | ||
| .groupBy('msce.referrer_source'); | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Possible double counting in MRR subquery
Grouping only by msce.referrer_source while joining members_paid_subscription_events can produce duplicate rows if multiple mpse rows share the same subscription/referrer. Add msce.subscription_id (or a surrogate unique key) to the GROUP BY, or wrap the join in a CTE that deduplicates per subscription before summing.
#!/bin/bash
Locate the PostsStatsService file
FILE=$(fd --exact-name PostsStatsService.js)
echo "Found file: $FILE"
Print lines around the MRR referrers subquery (approx lines 260–350)
sed -n '260,350p' "$FILE"
Show definition of the helper and its usage
rg -n "_buildMrrReferrersSubquery" -C3 "$FILE"
rg -n "mrr_referrers" -C5 "$FILE"
Ensure accurate MRR calculation in _buildMrrReferrersSubquery
The current implementation joins all members_paid_subscription_events for a subscription, so any subscription with multiple events (renewals, upgrades, churn) will have its mrr_delta counted multiple times when summed by referrer_source. You can address this in one of two ways:
- Filter to only the initial subscription‐created event:
let subquery = knex('members_subscription_created_events as msce')
.select('msce.referrer_source as source')
.sum('mpse.mrr_delta as mrr')
.join('members_paid_subscription_events as mpse', function () {
this.on('mpse.subscription_id', '=', 'msce.subscription_id')
.andOn('mpse.member_id', '=', 'msce.member_id');
})
.where('msce.attribution_id', postId)
.where('msce.attribution_type', 'post')
.andWhere('mpse.event', '=', 'subscription_created') // ← only count the initial event
.groupBy('msce.referrer_source');- Dedupe in a CTE (or subquery) so you only ever join a single
mpserow per subscription:
WITH initial_mrr AS (
SELECT subscription_id, mrr_delta
FROM members_paid_subscription_events
WHERE event = 'subscription_created'
)
SELECT
msce.referrer_source AS source,
SUM(im.mrr_delta) AS mrr
FROM members_subscription_created_events AS msce
JOIN initial_mrr AS im
ON im.subscription_id = msce.subscription_id
WHERE msce.attribution_id = :postId
AND msce.attribution_type = 'post'
GROUP BY msce.referrer_source;If you prefer to keep the single‐query style, at minimum add msce.subscription_id to the GROUP BY to avoid unintentionally aggregating duplicate rows:
.groupBy('msce.referrer_source', 'msce.subscription_id')[file: ghost/core/core/server/services/stats/PostsStatsService.js
Lines ~290–303]
| async function _createFreeSignup(postId, referrerSource,memberId = null) { | ||
| memberIdCounter += 1; | ||
| const finalMemberId = memberId || `member_${memberIdCounter}`; | ||
| await _createFreeSignupEvent(postId, finalMemberId); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
_createFreeSignup does not forward referrerSource, causing NULL values
_createFreeSignupEvent now expects a referrerSource, but the wrapper omits that argument.
Tests that rely on _createFreeSignup to populate the new column will silently insert NULL, which could hide regressions and skew referrer statistics if this helper is reused elsewhere.
async function _createFreeSignup(postId, referrerSource,memberId = null) {
memberIdCounter += 1;
const finalMemberId = memberId || `member_${memberIdCounter}`;
- await _createFreeSignupEvent(postId, finalMemberId);
+ await _createFreeSignupEvent(postId, finalMemberId, referrerSource);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function _createFreeSignup(postId, referrerSource,memberId = null) { | |
| memberIdCounter += 1; | |
| const finalMemberId = memberId || `member_${memberIdCounter}`; | |
| await _createFreeSignupEvent(postId, finalMemberId); | |
| } | |
| async function _createFreeSignup(postId, referrerSource,memberId = null) { | |
| memberIdCounter += 1; | |
| const finalMemberId = memberId || `member_${memberIdCounter}`; | |
| await _createFreeSignupEvent(postId, finalMemberId, referrerSource); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/posts/src/views/PostAnalytics/Growth.tsx (1)
73-81: Invalid URL & favicon fetch whenrow.sourceis falsey
href={https://${row.source}}and the favicon call both misbehave on an empty value (e.g., "Direct" traffic is typically stored asnull). Either:
- Filter out null sources in the query (backend), or
- Handle them gracefully in the UI:
-<a className='inline-flex items-center gap-2 font-medium' href={`https://${row.source}`} rel="noreferrer" target='_blank'> +<a + className='inline-flex items-center gap-2 font-medium' + href={row.source ? `https://${row.source}` : undefined} + rel="noopener noreferrer" + target={row.source ? '_blank' : undefined} +>Also add
noopenerto therelattribute for security.
🧹 Nitpick comments (1)
apps/posts/src/views/PostAnalytics/Growth.tsx (1)
10-11: Replace empty interface with a type alias or remove if unused.The empty interface
postAnalyticsPropsdoesn't add any value and is equivalent to{}. If you don't need to define props for this component, you can simplify it.-interface postAnalyticsProps {} - -const Growth: React.FC<postAnalyticsProps> = () => { +const Growth: React.FC = () => {Alternatively, if you plan to add props later, use a type alias for better clarity:
-interface postAnalyticsProps {} +type PostAnalyticsProps = { + // future props will go here +}; + +const Growth: React.FC<PostAnalyticsProps> = () => {Also, consider following TypeScript naming conventions using PascalCase for interfaces and type aliases.
🧰 Tools
🪛 Biome (1.9.4)
[error] 10-11: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/posts/src/views/PostAnalytics/Growth.tsx(3 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
apps/posts/src/views/PostAnalytics/Growth.tsx
[error] 10-11: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Unit tests (Node 22.13.1)
- GitHub Check: Regression tests (Node 20.11.1, sqlite3)
- GitHub Check: Unit tests (Node 20.11.1)
- GitHub Check: Regression tests (Node 20.11.1, mysql8)
- GitHub Check: Database tests (Node 20.11.1, sqlite3)
- GitHub Check: Database tests (Node 22.13.1, mysql8)
- GitHub Check: Database tests (Node 20.11.1, mysql8)
- GitHub Check: Admin-X Settings tests
- GitHub Check: i18n
- GitHub Check: Lint
🔇 Additional comments (2)
apps/posts/src/views/PostAnalytics/Growth.tsx (2)
7-8: Good implementation of dynamic post ID with hooks.The code now properly gets the post ID from URL parameters instead of using a hardcoded value, which addresses the previous review issue. This makes the component reusable across different posts and testable in isolation.
Also applies to: 15-16
36-36: Properly integrated dynamic analytics data in KPI cards.The KPI cards now correctly display dynamic data from the API instead of hardcoded values, making the analytics dashboard show real data for each post.
Also applies to: 43-43, 50-50
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ghost/core/test/e2e-api/admin/stats.test.js (1)
249-264: Good basic test coverage, consider enhancing data validationThe test successfully verifies that the new
/stats/posts/:id/top-referrersendpoint exists and returns a properly formatted response with a stats array. This aligns well with the PR objective of adding a new attribution endpoint.Consider enhancing this test to validate the actual structure of the returned data, such as verifying expected fields (source, free_members, paid_members, etc.) and expected values based on fixture data. Additionally, testing filter parameters (date ranges) and error handling scenarios (invalid post ID) would strengthen the test coverage.
You could extend the test to validate the structure with something like:
.expect(({body}) => { assert.ok(body.stats, 'Response should contain a stats property'); assert.ok(Array.isArray(body.stats), 'body.stats should be an array'); // Validate data structure if any referrers exist if (body.stats.length > 0) { const referrer = body.stats[0]; assert.ok('source' in referrer, 'Referrer should contain a source property'); assert.ok('free_members' in referrer, 'Referrer should contain a free_members property'); assert.ok('paid_members' in referrer, 'Referrer should contain a paid_members property'); } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
ghost/core/test/e2e-api/admin/__snapshots__/stats.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (3)
apps/admin-x-framework/src/api/stats.ts(3 hunks)ghost/core/core/server/web/api/endpoints/admin/routes.js(2 hunks)ghost/core/test/e2e-api/admin/stats.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- ghost/core/core/server/web/api/endpoints/admin/routes.js
- apps/admin-x-framework/src/api/stats.ts
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: Database tests (Node 20.11.1, sqlite3)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Regression tests (Node 20.11.1, sqlite3)
- GitHub Check: Database tests (Node 22.13.1, mysql8)
- GitHub Check: Regression tests (Node 20.11.1, mysql8)
- GitHub Check: Database tests (Node 20.11.1, mysql8)
- GitHub Check: Unit tests (Node 22.13.1)
- GitHub Check: Admin-X Settings tests
- GitHub Check: Unit tests (Node 20.11.1)
- GitHub Check: Lint
- GitHub Check: i18n
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/posts/src/views/PostAnalytics/Growth.tsx (1)
73-73:⚠️ Potential issueInvalid URL construction when row.source is falsey
The
href={https://${row.source}}will create invalid URLs for direct traffic (where source is null/empty) or sources that already include the protocol.-<a className='inline-flex items-center gap-2 font-medium' href={`https://${row.source}`} rel="noreferrer" target='_blank'> +<a + className='inline-flex items-center gap-2 font-medium' + href={row.source ? `https://${row.source}` : undefined} + rel="noopener noreferrer" + target={row.source ? '_blank' : undefined} +>Note: Added
noopenertorelfor security when opening external links.
🧹 Nitpick comments (2)
apps/posts/src/views/PostAnalytics/Growth.tsx (2)
11-11: Replace empty interface with more specific type or remove entirelyThe empty interface
postAnalyticsPropsdoesn't provide any value and is flagged by static analysis.-interface postAnalyticsProps {} +type PostAnalyticsProps = Record<string, never>;Or simply:
-interface postAnalyticsProps {} -const Growth: React.FC<postAnalyticsProps> = () => { +const Growth: React.FC = () => {🧰 Tools
🪛 Biome (1.9.4)
[error] 10-11: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
28-29: Add better loading stateThe current loading state just shows "Loading" text. Consider using a proper loading skeleton or spinner component for a better user experience.
-{isLoading ? 'Loading' : +{isLoading ? <LoadingPlaceholder /> :Where
LoadingPlaceholdercould be a component that displays skeleton UI elements matching the layout of the loaded data.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/posts/src/views/PostAnalytics/Growth.tsx(3 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
apps/posts/src/views/PostAnalytics/Growth.tsx
[error] 10-11: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
🔇 Additional comments (3)
apps/posts/src/views/PostAnalytics/Growth.tsx (3)
7-8: Good implementation of dynamic data fetchingThe component now properly retrieves data based on the post ID from URL parameters instead of using hardcoded values. This makes the component reusable across different posts and testable in isolation.
Also applies to: 15-16
36-36: Dynamic KPI card values correctly implementedKPI cards now display actual metrics from the API response, with proper formatting applied.
Also applies to: 43-43, 50-50
70-71: Table population is correctly implemented with fallbacksGood job implementing fallbacks for direct traffic with
row.source || 'direct'for the favicon URL androw.source || 'Direct'for display text. The metrics are correctly formatted with appropriate prefixes.Also applies to: 76-76, 80-80, 83-85
cmraible
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
apps/posts/src/views/PostAnalytics/Growth.tsx (2)
76-76: Improve favicon handlingWhile you've added a fallback for direct traffic and have an error handler, consider implementing a more robust solution as suggested in the previous review. The current implementation still makes an external request that might fail or timeout.
Are there any reliable self-hosted options for favicon extraction?
73-73:⚠️ Potential issueInvalid URL construction when row.source is falsey
The URL construction
href={https://${row.source}}will fail whenrow.sourceis falsey (null/undefined), which commonly happens for direct traffic. This issue was pointed out in a previous review but hasn't been properly addressed.-<a className='inline-flex items-center gap-2 font-medium' href={`https://${row.source}`} rel="noreferrer" target='_blank'> +<a className='inline-flex items-center gap-2 font-medium' + href={row.source ? `https://${row.source}` : undefined} + rel="noreferrer noopener" + target={row.source ? '_blank' : undefined}>
🧹 Nitpick comments (2)
apps/posts/src/views/PostAnalytics/Growth.tsx (2)
50-50: Consider using dynamic data for KPI valuesWhile you've correctly implemented the formatting for the MRR value, you're still using a hardcoded value (180). Consider using data from the API response to display actual MRR metrics.
-<KpiCardValue>+${formatNumber(180)}</KpiCardValue> +<KpiCardValue>+${formatNumber(totals?.mrr || 0)}</KpiCardValue>
11-11: Empty interface could be simplifiedThe
postAnalyticsPropsinterface is empty which is equivalent to{}. Consider using a type alias instead or removing it if no props are needed.-interface postAnalyticsProps {} +type PostAnalyticsProps = Record<string, never>;Or simply:
-interface postAnalyticsProps {} -const Growth: React.FC<postAnalyticsProps> = () => { +const Growth: React.FC = () => {🧰 Tools
🪛 Biome (1.9.4)
[error] 10-11: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/posts/src/hooks/usePostReferrers.ts(1 hunks)apps/posts/src/views/PostAnalytics/Growth.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/posts/src/hooks/usePostReferrers.ts
🧰 Additional context used
🪛 Biome (1.9.4)
apps/posts/src/views/PostAnalytics/Growth.tsx
[error] 10-11: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
🔇 Additional comments (5)
apps/posts/src/views/PostAnalytics/Growth.tsx (5)
7-8: Good implementation of dynamic data fetchingNice work importing the necessary hooks to fetch post analytics data dynamically. This properly implements the data fetching for the top referrers endpoint.
15-16: Properly fixed hardcoded post IDGreat job replacing the hardcoded post ID with the
useParams()hook. This addresses the previous review comment and makes the component properly handle dynamic post IDs.
70-71: Good implementation of dynamic data mappingNice work replacing mock data with actual post referrers data from the API.
80-80: Good handling of null sourceNice work adding a fallback display text for direct traffic when source is null or undefined.
83-85: Well-formatted metrics displayGood job using the
formatNumberutility for consistent formatting of metrics in the table.
cmraible
deleted the
chris-prod-1542-adjust-conversion-attribution-to-reflect-free-members-paid
branch
ref https://linear.app/ghost/issue/PROD-1542/adjust-conversion-attribution-to-reflect-free-members-and-paid-members
On the new Post Analytics page, we display a table of the top sources for a particular post, sorted by free members, paid members and MRR. This commit adds the API endpoint for this data, and wires it up to the frontend.