Added materialized view and duplicated v2 Tinybird endpoints#51
Conversation
…re robust polling approach
WalkthroughThis pull request introduces Tinybird v2 analytics endpoints with a versioned endpoint system. It adds a new materialized view ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ghost/core/core/server/services/stats/ContentStatsService.js (1)
75-122: Add missingsourceparameter mapping totinybirdOptions.The test case at line 524 of
content.test.jsexplicitly expectstinybirdOptions.should.have.property('source', 'google.com'), but the source parameter is not being mapped infetchRawTopContentData(). Add the following after line 99 to match the pattern used for other optional parameters:// Only add source if defined if (options.source) { tinybirdOptions.source = options.source; }
🤖 Fix all issues with AI agents
In `@apps/admin-x-framework/src/providers/framework-provider.tsx`:
- Line 90: Replace the usage of FrameworkContext.Provider that passes children
as a prop: remove the children prop and render children between the Provider
tags instead; update the JSX where FrameworkContext.Provider is used (the
element currently written as FrameworkContext.Provider with children={children}
and value={props}) to use <FrameworkContext.Provider
value={props}>children</FrameworkContext.Provider> style so children are not
passed as a prop.
In `@apps/admin-x-framework/src/utils/stats-config.ts`:
- Around line 17-20: The code builds versioned endpoint names by prefixing the
version (const finalEndpointName = config.version ?
`${config.version}_${endpointName}` : endpointName) but v2 pipes use a suffix
(e.g., api_top_utm_sources_v2); update the logic in stats-config (where
finalEndpointName is computed) to append the version as a suffix when
config.version is present and adjust the comment to say "append version suffix"
so finalEndpointName becomes `${endpointName}_${config.version}` for versioned
endpoints.
In `@ghost/core/core/server/data/tinybird/endpoints/api_top_devices_v2.pipe`:
- Around line 18-24: The join between session_data (sd) and filtered_sessions_v2
(fs) only matches on session_id which can cross tenants; update the ON clause in
the query (the inner join between filtered_sessions_v2 fs and session_data sd)
to include fs.site_uuid = sd.site_uuid in addition to fs.session_id =
sd.session_id so rows are matched per-tenant; no other structural changes
required unless you also want to surface site_uuid in SELECT/GROUP BY for
downstream use.
In `@ghost/core/core/server/data/tinybird/tests/api_top_locations_v2.yaml`:
- Around line 2-86: The SQL used by these tests currently sorts results with
"ORDER BY visits DESC" which produces non-deterministic order for ties; locate
the query that appears in the api_top_locations_v2 test (search for the "ORDER
BY visits DESC" clause) and change it to "ORDER BY visits DESC, location" so
tied visit counts are deterministically ordered by location (update any
occurrences in the query builder or raw SQL used by the api_top_locations_v2
test cases).
In `@ghost/core/core/server/data/tinybird/tests/api_top_utm_campaigns_v2.yaml`:
- Around line 2-79: Tests for api_top_utm_campaigns_v2 are flaky because the
query only uses "order by visits desc"; update the endpoint/query
(api_top_utm_campaigns_v2) to add a deterministic secondary sort by campaign
name by changing the ORDER BY clause from "order by visits desc" to "order by
visits desc, utm_campaign asc" wherever that clause appears so tied visit counts
are consistently ordered.
In `@ghost/core/core/server/data/tinybird/tests/api_top_utm_mediums_v2.yaml`:
- Around line 2-79: The SQL ordering is non-deterministic because the endpoint
uses "order by visits desc" with tied visit counts; update the endpoint's ORDER
BY clause to add a deterministic secondary key so results sort consistently
(e.g., change "order by visits desc" to "ORDER BY visits DESC, utm_medium ASC")
wherever the query is defined (the ORDER BY clause / "order by visits desc" in
the Tinybird endpoint or query builder), and ensure any code that constructs
ordering uses the same deterministic ordering so the YAML fixtures no longer
flake.
🧹 Nitpick comments (1)
ghost/core/core/server/data/tinybird/tests/api_active_visitors_v2.yaml (1)
2-12: Consider consolidating duplicate test cases.The
default_site_uuidandactive_visitors_defaulttests are identical — same parameters (site_uuid=mock_site_uuid) and same expected result ({"active_visitors":16}). Consider removing one or differentiating them if they're intended to test distinct scenarios.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (36)
apps/admin-x-framework/src/providers/framework-provider.tsxapps/admin-x-framework/src/utils/stats-config.tsghost/core/core/server/api/endpoints/stats.jsghost/core/core/server/data/tinybird/README.mdghost/core/core/server/data/tinybird/datasources/_mv_session_data_v2.datasourceghost/core/core/server/data/tinybird/endpoints/README.mdghost/core/core/server/data/tinybird/endpoints/api_active_visitors_v2.pipeghost/core/core/server/data/tinybird/endpoints/api_kpis_v2.pipeghost/core/core/server/data/tinybird/endpoints/api_post_visitor_counts_v2.pipeghost/core/core/server/data/tinybird/endpoints/api_top_devices_v2.pipeghost/core/core/server/data/tinybird/endpoints/api_top_locations_v2.pipeghost/core/core/server/data/tinybird/endpoints/api_top_pages_v2.pipeghost/core/core/server/data/tinybird/endpoints/api_top_sources_v2.pipeghost/core/core/server/data/tinybird/endpoints/api_top_utm_campaigns_v2.pipeghost/core/core/server/data/tinybird/endpoints/api_top_utm_contents_v2.pipeghost/core/core/server/data/tinybird/endpoints/api_top_utm_mediums_v2.pipeghost/core/core/server/data/tinybird/endpoints/api_top_utm_sources_v2.pipeghost/core/core/server/data/tinybird/endpoints/api_top_utm_terms_v2.pipeghost/core/core/server/data/tinybird/pipes/filtered_sessions_v2.pipeghost/core/core/server/data/tinybird/pipes/mv_session_data_v2.pipeghost/core/core/server/data/tinybird/tests/api_active_visitors_v2.yamlghost/core/core/server/data/tinybird/tests/api_kpis_v2.yamlghost/core/core/server/data/tinybird/tests/api_post_visitor_counts_v2.yamlghost/core/core/server/data/tinybird/tests/api_top_devices_v2.yamlghost/core/core/server/data/tinybird/tests/api_top_locations_v2.yamlghost/core/core/server/data/tinybird/tests/api_top_pages_v2.yamlghost/core/core/server/data/tinybird/tests/api_top_sources_v2.yamlghost/core/core/server/data/tinybird/tests/api_top_utm_campaigns_v2.yamlghost/core/core/server/data/tinybird/tests/api_top_utm_contents_v2.yamlghost/core/core/server/data/tinybird/tests/api_top_utm_mediums_v2.yamlghost/core/core/server/data/tinybird/tests/api_top_utm_sources_v2.yamlghost/core/core/server/data/tinybird/tests/api_top_utm_terms_v2.yamlghost/core/core/server/services/stats/ContentStatsService.jsghost/core/core/server/services/stats/utils/tinybird.jsghost/core/core/server/services/tinybird/TinybirdService.jsghost/core/test/unit/server/services/stats/utils/tinybird.test.js
💤 Files with no reviewable changes (1)
- ghost/core/core/server/api/endpoints/stats.js
🧰 Additional context used
🧬 Code graph analysis (2)
ghost/core/core/server/services/stats/ContentStatsService.js (1)
ghost/core/test/unit/server/services/stats/content.test.js (6)
options(64-67)options(324-327)options(352-355)options(399-399)options(458-463)options(494-507)
ghost/core/core/server/services/stats/utils/tinybird.js (1)
ghost/core/core/server/services/public-config/config.js (1)
statsConfig(33-33)
🪛 Biome (2.3.13)
apps/admin-x-framework/src/providers/framework-provider.tsx
[error] 90-90: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
🔇 Additional comments (27)
ghost/core/core/server/data/tinybird/README.md (1)
81-82: LGTM!Clear and helpful documentation of the new optional
versionfield. The example correctly illustrates how to enable v2 endpoints via configuration.ghost/core/core/server/services/tinybird/TinybirdService.js (1)
59-72: LGTM!The v2 pipe names are correctly added, maintaining a consistent 1:1 mapping with the v1 pipes. The comment clarifying the materialized view optimization is helpful.
ghost/core/core/server/data/tinybird/endpoints/api_active_visitors_v2.pipe (1)
1-15: LGTM!The endpoint is well-structured:
- Uses the materialized view
_mv_hitsas intended for v2 optimization.- Properly typed parameters with
String()prevent injection risks.- The 5-minute window is appropriate for active visitors tracking.
- Optional
post_uuidfilter is correctly implemented with conditional templating.ghost/core/core/server/data/tinybird/endpoints/api_post_visitor_counts_v2.pipe (1)
7-15: The v1 endpoint (api_post_visitor_counts.pipe) has an identical query structure—no date filtering applied. This confirms the absence of time-based filtering is an intentional design choice, not an oversight. The endpoint correctly returns all-time visitor counts per post. This is distinct fromapi_active_visitors_v2, which tracks recent active visitors; they serve different purposes.ghost/core/core/server/data/tinybird/tests/api_post_visitor_counts_v2.yaml (1)
2-19: Good coverage for single, multi, and empty cases.The fixtures are clear and exercise the expected result shapes.
ghost/core/test/unit/server/services/stats/utils/tinybird.test.js (1)
73-87: Test update matches config-driven versioning.ghost/core/core/server/data/tinybird/datasources/_mv_session_data_v2.datasource (1)
1-16: Schema and sorting key look consistent for AggregatingMergeTree.ghost/core/core/server/data/tinybird/pipes/mv_session_data_v2.pipe (1)
1-22: MV pipe aligns with the v2 session datasource.ghost/core/core/server/data/tinybird/pipes/filtered_sessions_v2.pipe (1)
3-85: Two-stage filtering is clear and MV usage is consistent.ghost/core/core/server/services/stats/utils/tinybird.js (1)
35-40: No issues identified. All callers pass unversioned pipe names (api_top_pages,api_post_visitor_counts), and no code passestbVersionor version parameters to the tinybird client. Version from config is correctly applied only to the pipe name in the URL, not as a query parameter. The implementation is secure.Likely an incorrect or invalid review comment.
ghost/core/core/server/data/tinybird/endpoints/api_top_sources_v2.pipe (1)
1-28: LGTM - Consistent v2 endpoint pattern.The endpoint correctly uses
argMinMergeto extract the first source per session from the materialized view and joins withfiltered_sessions_v2for additional filtering. The absence of an empty-source filter (unlike the UTM endpoints) appears intentional since empty source typically represents direct traffic.ghost/core/core/server/data/tinybird/endpoints/api_top_utm_contents_v2.pipe (1)
1-30: LGTM - Well-structured UTM content endpoint.The endpoint follows the established v2 pattern: extracting session-level UTM content via
argMinMerge, joining with the filtered sessions pipe, filtering out empty values, and applying pagination. Implementation is consistent with sibling UTM endpoints.ghost/core/core/server/data/tinybird/tests/api_top_utm_contents_v2.yaml (1)
1-81: Comprehensive test coverage for UTM contents endpoint.Good coverage across all filter dimensions: location, pathname, post_uuid, source, member_status, timezone, and combined filters. The test scenarios align well with the endpoint's capabilities.
ghost/core/core/server/data/tinybird/endpoints/api_top_utm_terms_v2.pipe (1)
1-30: LGTM - Consistent with UTM endpoint pattern.The endpoint mirrors the structure of other UTM endpoints (contents, mediums, etc.) with appropriate field substitution for
utm_term. Empty value filtering and pagination are correctly implemented.ghost/core/core/server/data/tinybird/tests/api_top_utm_terms_v2.yaml (1)
1-82: Well-structured test suite matching other v2 endpoint tests.Test coverage mirrors the utm_contents tests with appropriate utm_term-specific expected values. The consistent test structure across v2 endpoints aids maintainability.
ghost/core/core/server/data/tinybird/endpoints/api_top_utm_mediums_v2.pipe (1)
1-30: LGTM - Completes the UTM endpoint family.The endpoint follows the established v2 UTM pattern with correct field handling for
utm_medium. Structure is consistent with the other UTM dimension endpoints (sources, campaigns, contents, terms).ghost/core/core/server/data/tinybird/endpoints/api_top_pages_v2.pipe (1)
12-14: The join is safe as-is and does not require modification.The WHERE clause in api_top_pages_v2 filters
_mv_hitsto the requesting tenant (site_uuid = {{site_uuid}}), andfiltered_sessions_v2pre-filters sessions bysite_uuidinternally before selection. Both sides of the join are already isolated to the same tenant, so tenant isolation is protected. Additionally,filtered_sessions_v2outputs onlysession_id(notsite_uuid), making the suggested fix impossible to implement. This pattern is consistent across all v2 endpoints and is the correct design.Likely an incorrect or invalid review comment.
ghost/core/core/server/data/tinybird/endpoints/api_top_utm_campaigns_v2.pipe (1)
21-24: The suggested join fix is infeasible, but cross-tenant isolation concern should be addressed differently.The concern about
session_iduniqueness is valid—the schema showssite_uuidandsession_idform a composite key. However, the proposed fix cannot be applied as-is becausefiltered_sessions_v2outputs onlysession_id, notsite_uuid.Both
session_dataandfiltered_sessions_v2are already filtered by the samesite_uuidparameter, providing runtime tenant isolation. However, this relies on correct parameterization rather than schema-level enforcement.Better approaches:
- Modify
filtered_sessions_v2to includesite_uuidin its output and add it to the join condition, or- Add an explicit
WHERE site_uuid = {{ String(site_uuid) }}filter at the endpoint level (as done in v1), or- Document that tenant safety depends on consistent parameter handling between the two nodes.
ghost/core/core/server/data/tinybird/endpoints/api_top_locations_v2.pipe (1)
11-13: The join pattern is correct and does not introduce cross-tenant collision risk. In Tinybird, endpoint parameters are automatically passed to referenced nodes. Sincefiltered_sessions_v2is called as a referenced node, it receives the samesite_uuidparameter as the parent endpoint and filters its internal queries accordingly (lines 14 and 58 infiltered_sessions_v2.pipe). Both_mv_hitsandfiltered_sessions_v2are therefore scoped to the same tenant before the join, making an explicitsite_uuidjoin condition redundant. This pattern is consistent across all v2 endpoints and matches the v1 endpoint design.Likely an incorrect or invalid review comment.
ghost/core/core/server/data/tinybird/tests/api_kpis_v2.yaml (1)
2-267: Nice breadth of fixtures and filter combinations.Coverage looks comprehensive for date ranges, timezones, and filters.
ghost/core/core/server/data/tinybird/tests/api_top_sources_v2.yaml (1)
2-131: Solid coverage across filters and UTM dimensions.ghost/core/core/server/data/tinybird/tests/api_top_pages_v2.yaml (1)
2-135: Comprehensive fixture coverage for top pages v2.ghost/core/core/server/data/tinybird/tests/api_top_devices_v2.yaml (1)
2-62: Test coverage looks solid for device filters and segments.ghost/core/core/server/data/tinybird/endpoints/api_kpis_v2.pipe (1)
81-96: This join pattern is safe due to existing parameter-based tenant filtering.Both
session_dataandfiltered_sessions_v2are already filtered bysite_uuidparameter in their WHERE clauses before the join occurs. The data model (MergeTree sorting keys includesite_uuid, session_id) ensures tenant isolation at the table level. Adding site_uuid to the join predicate is not necessary and the proposed fix is not feasible—filtered_sessions_v2only exposessession_idin its final output, notsite_uuid.ghost/core/core/server/data/tinybird/tests/api_top_utm_sources_v2.yaml (1)
2-88: Good coverage for v2 UTM source filters.The fixture cases cover baseline and key filter combinations, which should help catch regressions.
ghost/core/core/server/data/tinybird/endpoints/README.md (1)
9-42: Documentation updates are clear and consistent.The revised session-level sourcing and filtering stages accurately describe the v2 data flow.
ghost/core/core/server/data/tinybird/endpoints/api_top_utm_sources_v2.pipe (1)
4-29: Endpoint logic aligns with v2 session aggregation pattern.Session-level UTM extraction plus filtered session join looks consistent with the v2 design.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| <FrameworkContext.Provider value={props}> | ||
| {children} | ||
| </FrameworkContext.Provider> | ||
| <FrameworkContext.Provider children={children} value={props} /> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the actual code at the specified location
head -n 95 apps/admin-x-framework/src/providers/framework-provider.tsx | tail -n 10Repository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 333
🏁 Script executed:
# Check for Biome configuration
find . -name "biome.json" -o -name "biome.jsonc" -o -name ".biomeignore"Repository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 96
🏁 Script executed:
# Search for lint rules in the codebase
rg "noChildrenProp" --type jsonRepository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 66
🏁 Script executed:
# Read the Biome configuration
cat biome.jsonRepository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 1859
🏁 Script executed:
# Search for any lint configuration related to children or correctness
rg "children" biome.json -A 3 -B 3Repository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 66
🌐 Web query:
Biome lint rule noChildrenProp jsx children prop
💡 Result:
Biome’s lint rule is lint/correctness/noChildrenProp (rule name: noChildrenProp). It disallows passing children via a JSX prop, e.g. <MyComp children={...} />, because the canonical React pattern is to pass children between tags (or as additional args to React.createElement). It’s recommended and enabled by default, and Biome reports it as an error. [1]
What it flags
// ❌ flagged
<FirstComponent children={'foo'} />
// ❌ flagged
React.createElement('div', { children: 'foo' });[1]
What to do instead
// ✅ preferred
<FirstComponent>{'foo'}</FirstComponent>
// ✅ preferred
React.createElement('div', {}, 'foo');[1]
Config (if you want to explicitly set it)
{
"linter": {
"rules": {
"correctness": {
"noChildrenProp": "error"
}
}
}
}[1]
If you’re trying to allow function-as-children patterns, note that the upstream ESLint rule (react/no-children-prop) documents an allowFunctions option (Biome may not expose that option the same way). [2]
Sources: Biome rule docs [1]; upstream ESLint rule behavior/options [2].
[1] https://biomejs.dev/linter/rules/no-children-prop/
[2] https://help.klocwork.com/2024/en-us/reference/js.react.no.children.prop.htm
Avoid passing children as a prop—Biome lint error.
The noChildrenProp rule is enabled by default (via "recommended": true in biome.json) and will fail linting. Pass children between tags instead.
🔧 Proposed fix
- <FrameworkContext.Provider children={children} value={props} />
+ <FrameworkContext.Provider value={props}>
+ {children}
+ </FrameworkContext.Provider>📝 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.
| <FrameworkContext.Provider children={children} value={props} /> | |
| <FrameworkContext.Provider value={props}> | |
| {children} | |
| </FrameworkContext.Provider> |
🧰 Tools
🪛 Biome (2.3.13)
[error] 90-90: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
🤖 Prompt for AI Agents
In `@apps/admin-x-framework/src/providers/framework-provider.tsx` at line 90,
Replace the usage of FrameworkContext.Provider that passes children as a prop:
remove the children prop and render children between the Provider tags instead;
update the JSX where FrameworkContext.Provider is used (the element currently
written as FrameworkContext.Provider with children={children} and value={props})
to use <FrameworkContext.Provider
value={props}>children</FrameworkContext.Provider> style so children are not
passed as a prop.
| // Append version suffix if provided (e.g., "v2" -> "api_kpis_v2") | ||
| const finalEndpointName = config.version ? `${config.version}_${endpointName}` : endpointName; | ||
|
|
||
| return `${baseUrl}/v0/pipes/${finalEndpointName}.json?${params}`; |
There was a problem hiding this comment.
Fix versioned endpoint naming to match v2 pipe suffixes.
Current logic builds v2_<endpointName>, but the v2 pipes in this PR are suffixed (e.g., api_top_utm_sources_v2). With config.version=v2 and endpointName=api_top_utm_sources, this will point at a non-existent pipe. Also the comment says “suffix” but the code prefixes.
🔧 Proposed fix
- // Append version suffix if provided (e.g., "v2" -> "api_kpis_v2")
- const finalEndpointName = config.version ? `${config.version}_${endpointName}` : endpointName;
+ // Append version suffix if provided (e.g., "v2" -> "api_kpis_v2")
+ const finalEndpointName = config.version ? `${endpointName}_${config.version}` : endpointName;📝 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.
| // Append version suffix if provided (e.g., "v2" -> "api_kpis_v2") | |
| const finalEndpointName = config.version ? `${config.version}_${endpointName}` : endpointName; | |
| return `${baseUrl}/v0/pipes/${finalEndpointName}.json?${params}`; | |
| // Append version suffix if provided (e.g., "v2" -> "api_kpis_v2") | |
| const finalEndpointName = config.version ? `${endpointName}_${config.version}` : endpointName; | |
| return `${baseUrl}/v0/pipes/${finalEndpointName}.json?${params}`; |
🤖 Prompt for AI Agents
In `@apps/admin-x-framework/src/utils/stats-config.ts` around lines 17 - 20, The
code builds versioned endpoint names by prefixing the version (const
finalEndpointName = config.version ? `${config.version}_${endpointName}` :
endpointName) but v2 pipes use a suffix (e.g., api_top_utm_sources_v2); update
the logic in stats-config (where finalEndpointName is computed) to append the
version as a suffix when config.version is present and adjust the comment to say
"append version suffix" so finalEndpointName becomes
`${endpointName}_${config.version}` for versioned endpoints.
| select | ||
| device, | ||
| count() as visits | ||
| from session_data sd | ||
| inner join filtered_sessions_v2 fs | ||
| on fs.session_id = sd.session_id | ||
| group by device |
There was a problem hiding this comment.
Include site_uuid in the filtered_sessions_v2 join.
Joining only on session_id can mix sessions across tenants if IDs collide. Safer to join on both session_id and site_uuid.
🔧 Proposed fix
- from session_data sd
- inner join filtered_sessions_v2 fs
- on fs.session_id = sd.session_id
+ from session_data sd
+ inner join filtered_sessions_v2 fs
+ on fs.session_id = sd.session_id
+ and fs.site_uuid = sd.site_uuid📝 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.
| select | |
| device, | |
| count() as visits | |
| from session_data sd | |
| inner join filtered_sessions_v2 fs | |
| on fs.session_id = sd.session_id | |
| group by device | |
| select | |
| device, | |
| count() as visits | |
| from session_data sd | |
| inner join filtered_sessions_v2 fs | |
| on fs.session_id = sd.session_id | |
| and fs.site_uuid = sd.site_uuid | |
| group by device |
🤖 Prompt for AI Agents
In `@ghost/core/core/server/data/tinybird/endpoints/api_top_devices_v2.pipe`
around lines 18 - 24, The join between session_data (sd) and
filtered_sessions_v2 (fs) only matches on session_id which can cross tenants;
update the ON clause in the query (the inner join between filtered_sessions_v2
fs and session_data sd) to include fs.site_uuid = sd.site_uuid in addition to
fs.session_id = sd.session_id so rows are matched per-tenant; no other
structural changes required unless you also want to surface site_uuid in
SELECT/GROUP BY for downstream use.
| - name: Date range | ||
| description: All fixture data | ||
| parameters: site_uuid=mock_site_uuid&date_from=2100-01-01&date_to=2100-01-07&timezone=Etc/UTC | ||
| expected_result: | | ||
| {"location":"GB","visits":8} | ||
| {"location":"US","visits":3} | ||
| {"location":"FR","visits":2} | ||
| {"location":"ES","visits":2} | ||
| {"location":"DE","visits":1} | ||
|
|
||
| - name: Filtered by location - UK | ||
| description: Filtered by location - UK | ||
| parameters: site_uuid=mock_site_uuid&date_from=2100-01-01&date_to=2100-01-07&timezone=Etc/UTC&location=GB | ||
| expected_result: | | ||
| {"location":"GB","visits":8} | ||
|
|
||
| - name: Filtered by pathname - /about/ | ||
| description: Filtered by pathname - /about/ | ||
| parameters: site_uuid=mock_site_uuid&date_from=2100-01-01&date_to=2100-01-07&timezone=Etc/UTC&pathname=%2Fabout%2F | ||
| expected_result: | | ||
| {"location":"GB","visits":4} | ||
| {"location":"FR","visits":1} | ||
| {"location":"US","visits":1} | ||
| {"location":"DE","visits":1} | ||
| {"location":"ES","visits":1} | ||
|
|
||
| - name: Filtered by post_uuid - 06b1b0c9-fb53-4a15-a060-3db3fde7b1fc (/about/) | ||
| description: Filtered by post_uuid - 06b1b0c9-fb53-4a15-a060-3db3fde7b1fc (/about/) | ||
| parameters: site_uuid=mock_site_uuid&date_from=2100-01-01&date_to=2100-01-07&timezone=Etc/UTC&post_uuid=06b1b0c9-fb53-4a15-a060-3db3fde7b1fc | ||
| expected_result: | | ||
| {"location":"GB","visits":4} | ||
| {"location":"FR","visits":1} | ||
| {"location":"US","visits":1} | ||
| {"location":"DE","visits":1} | ||
| {"location":"ES","visits":1} | ||
|
|
||
| - name: Filtered by source - bing.com | ||
| description: Filtered by source - bing.com | ||
| parameters: site_uuid=mock_site_uuid&date_from=2100-01-01&date_to=2100-01-07&timezone=Etc/UTC&source=bing.com | ||
| expected_result: | | ||
| {"location":"DE","visits":1} | ||
| {"location":"GB","visits":1} | ||
|
|
||
| - name: Filtered by member status - paid | ||
| description: Filtered by member status - paid | ||
| parameters: site_uuid=mock_site_uuid&date_from=2100-01-01&date_to=2100-01-07&timezone=Etc/UTC&member_status=paid | ||
| expected_result: | | ||
| {"location":"GB","visits":2} | ||
| {"location":"FR","visits":1} | ||
| {"location":"DE","visits":1} | ||
| {"location":"ES","visits":1} | ||
|
|
||
| - name: Filtered by member status - undefined | ||
| description: Filtered by member status - undefined | ||
| parameters: site_uuid=mock_site_uuid&date_from=2100-01-01&date_to=2100-01-07&timezone=Etc/UTC&member_status=undefined | ||
| expected_result: | | ||
| {"location":"GB","visits":4} | ||
| {"location":"US","visits":2} | ||
|
|
||
| - name: Filtered by timezone - America/Los_Angeles | ||
| description: Filtered by timezone - America/Los_Angeles | ||
| parameters: site_uuid=mock_site_uuid&date_from=2100-01-01&date_to=2100-01-07&timezone=America/Los_Angeles | ||
| expected_result: | | ||
| {"location":"GB","visits":6} | ||
| {"location":"US","visits":3} | ||
| {"location":"FR","visits":2} | ||
| {"location":"DE","visits":1} | ||
| {"location":"ES","visits":1} | ||
|
|
||
| - name: Test with multiple filters combined | ||
| description: Test with multiple filters combined | ||
| parameters: site_uuid=mock_site_uuid&date_from=2100-01-01&date_to=2100-01-07&timezone=Etc/UTC&source=bing.com&pathname=%2Fabout%2F | ||
| expected_result: | | ||
| {"location":"DE","visits":1} | ||
| {"location":"GB","visits":1} | ||
|
|
||
| - name: Filtered by device - desktop | ||
| description: Filtered by device - desktop (excludes bot session) | ||
| parameters: site_uuid=mock_site_uuid&date_from=2100-01-01&date_to=2100-01-07&timezone=Etc/UTC&device=desktop | ||
| expected_result: | | ||
| {"location":"GB","visits":7} | ||
| {"location":"US","visits":3} | ||
| {"location":"FR","visits":2} | ||
| {"location":"ES","visits":2} | ||
| {"location":"DE","visits":1} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate and inspect api_top_locations_v2.pipe
find . -name "api_top_locations_v2.pipe" -type fRepository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 158
🏁 Script executed:
#!/bin/bash
# If found, read the file to check ORDER BY clause
find . -name "api_top_locations_v2.pipe" -type f -exec cat {} \;Repository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 1430
Add deterministic secondary ordering to prevent flaky tests with tied visit counts.
The current ORDER BY visits DESC lacks a secondary sort criterion. The test expectations contain multiple cases with tied visit counts (e.g., FR and ES both with visits=2 in the "Date range" test, and DE/GB/FR/ES/US with visits=1 in the "Filtered by pathname" test). Without stable ordering, the database may return tied records in any order, causing test failures. Add a secondary ORDER BY location to ensure deterministic results.
🤖 Prompt for AI Agents
In `@ghost/core/core/server/data/tinybird/tests/api_top_locations_v2.yaml` around
lines 2 - 86, The SQL used by these tests currently sorts results with "ORDER BY
visits DESC" which produces non-deterministic order for ties; locate the query
that appears in the api_top_locations_v2 test (search for the "ORDER BY visits
DESC" clause) and change it to "ORDER BY visits DESC, location" so tied visit
counts are deterministically ordered by location (update any occurrences in the
query builder or raw SQL used by the api_top_locations_v2 test cases).
| - name: Date range | ||
| description: All fixture data | ||
| parameters: site_uuid=mock_site_uuid&date_from=2100-01-01&date_to=2100-01-07&timezone=Etc/UTC | ||
| expected_result: | | ||
| {"utm_campaign":"brand_awareness","visits":2} | ||
| {"utm_campaign":"summer_sale_2024","visits":2} | ||
| {"utm_campaign":"holiday_promo","visits":2} | ||
| {"utm_campaign":"product_launch","visits":2} | ||
| {"utm_campaign":"retention_q4","visits":1} | ||
| {"utm_campaign":"newsletter_weekly","visits":1} | ||
|
|
||
| - name: Filtered by location - UK | ||
| description: Filtered by location - UK | ||
| parameters: site_uuid=mock_site_uuid&date_from=2100-01-01&date_to=2100-01-07&timezone=Etc/UTC&location=GB | ||
| expected_result: | | ||
| {"utm_campaign":"summer_sale_2024","visits":2} | ||
| {"utm_campaign":"brand_awareness","visits":1} | ||
| {"utm_campaign":"product_launch","visits":1} | ||
| {"utm_campaign":"newsletter_weekly","visits":1} | ||
|
|
||
| - name: Filtered by pathname - /about/ | ||
| description: Filtered by pathname - /about/ | ||
| parameters: site_uuid=mock_site_uuid&date_from=2100-01-01&date_to=2100-01-07&timezone=Etc/UTC&pathname=%2Fabout%2F | ||
| expected_result: | | ||
| {"utm_campaign":"product_launch","visits":2} | ||
| {"utm_campaign":"brand_awareness","visits":1} | ||
| {"utm_campaign":"summer_sale_2024","visits":1} | ||
| {"utm_campaign":"retention_q4","visits":1} | ||
| {"utm_campaign":"newsletter_weekly","visits":1} | ||
|
|
||
| - name: Filtered by post_uuid - 06b1b0c9-fb53-4a15-a060-3db3fde7b1fc (/about/) | ||
| description: Filtered by post_uuid - 06b1b0c9-fb53-4a15-a060-3db3fde7b1fc (/about/) | ||
| parameters: site_uuid=mock_site_uuid&date_from=2100-01-01&date_to=2100-01-07&timezone=Etc/UTC&post_uuid=06b1b0c9-fb53-4a15-a060-3db3fde7b1fc | ||
| expected_result: | | ||
| {"utm_campaign":"product_launch","visits":2} | ||
| {"utm_campaign":"brand_awareness","visits":1} | ||
| {"utm_campaign":"summer_sale_2024","visits":1} | ||
| {"utm_campaign":"retention_q4","visits":1} | ||
| {"utm_campaign":"newsletter_weekly","visits":1} | ||
|
|
||
| - name: Filtered by source - bing.com | ||
| description: Filtered by source - bing.com | ||
| parameters: site_uuid=mock_site_uuid&date_from=2100-01-01&date_to=2100-01-07&timezone=Etc/UTC&source=bing.com | ||
| expected_result: | | ||
| {"utm_campaign":"retention_q4","visits":1} | ||
| {"utm_campaign":"newsletter_weekly","visits":1} | ||
|
|
||
| - name: Filtered by member status - paid | ||
| description: Filtered by member status - paid | ||
| parameters: site_uuid=mock_site_uuid&date_from=2100-01-01&date_to=2100-01-07&timezone=Etc/UTC&member_status=paid | ||
| expected_result: | | ||
| {"utm_campaign":"retention_q4","visits":1} | ||
| {"utm_campaign":"product_launch","visits":1} | ||
|
|
||
| - name: Filtered by member status - undefined | ||
| description: Filtered by member status - undefined | ||
| parameters: site_uuid=mock_site_uuid&date_from=2100-01-01&date_to=2100-01-07&timezone=Etc/UTC&member_status=undefined | ||
| expected_result: | | ||
| {"utm_campaign":"summer_sale_2024","visits":2} | ||
| {"utm_campaign":"holiday_promo","visits":1} | ||
| {"utm_campaign":"product_launch","visits":1} | ||
|
|
||
| - name: Filtered by timezone - America/Los_Angeles | ||
| description: Filtered by timezone - America/Los_Angeles | ||
| parameters: site_uuid=mock_site_uuid&date_from=2100-01-01&date_to=2100-01-07&timezone=America/Los_Angeles | ||
| expected_result: | | ||
| {"utm_campaign":"holiday_promo","visits":2} | ||
| {"utm_campaign":"product_launch","visits":2} | ||
| {"utm_campaign":"brand_awareness","visits":1} | ||
| {"utm_campaign":"summer_sale_2024","visits":1} | ||
| {"utm_campaign":"retention_q4","visits":1} | ||
|
|
||
| - name: Test with multiple filters combined | ||
| description: Test with multiple filters combined | ||
| parameters: site_uuid=mock_site_uuid&date_from=2100-01-01&date_to=2100-01-07&timezone=Etc/UTC&source=bing.com&pathname=%2Fabout%2F | ||
| expected_result: | | ||
| {"utm_campaign":"retention_q4","visits":1} | ||
| {"utm_campaign":"newsletter_weekly","visits":1} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd 'api_top_utm_campaigns_v2\.pipe$' -aRepository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 179
🏁 Script executed:
cat /home/jailuser/git/ghost/core/core/server/data/tinybird/endpoints/api_top_utm_campaigns_v2.pipeRepository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 784
Add secondary ORDER BY for deterministic campaign ordering in results.
The endpoint's ORDER BY visits desc lacks a tie-breaker. Multiple test cases expect specific ordering for campaigns with identical visit counts (e.g., "Date range" has four campaigns with visits=2, "Filtered by timezone" has two with visits=2). Without a secondary sort, result order becomes non-deterministic and tests will be flaky.
Add utm_campaign as a secondary sort:
order by visits desc, utm_campaign asc
🤖 Prompt for AI Agents
In `@ghost/core/core/server/data/tinybird/tests/api_top_utm_campaigns_v2.yaml`
around lines 2 - 79, Tests for api_top_utm_campaigns_v2 are flaky because the
query only uses "order by visits desc"; update the endpoint/query
(api_top_utm_campaigns_v2) to add a deterministic secondary sort by campaign
name by changing the ORDER BY clause from "order by visits desc" to "order by
visits desc, utm_campaign asc" wherever that clause appears so tied visit counts
are consistently ordered.
| - name: Date range | ||
| description: All fixture data - real UTM mediums from mv_session_data | ||
| parameters: site_uuid=mock_site_uuid&date_from=2100-01-01&date_to=2100-01-07&timezone=Etc/UTC | ||
| expected_result: | | ||
| {"utm_medium":"social","visits":5} | ||
| {"utm_medium":"cpc","visits":2} | ||
| {"utm_medium":"organic","visits":1} | ||
| {"utm_medium":"referral","visits":1} | ||
| {"utm_medium":"display","visits":1} | ||
| {"utm_medium":"email","visits":1} | ||
|
|
||
| - name: Filtered by location - UK | ||
| description: Filtered by location - UK | ||
| parameters: site_uuid=mock_site_uuid&date_from=2100-01-01&date_to=2100-01-07&timezone=Etc/UTC&location=GB | ||
| expected_result: | | ||
| {"utm_medium":"cpc","visits":1} | ||
| {"utm_medium":"organic","visits":1} | ||
| {"utm_medium":"referral","visits":1} | ||
| {"utm_medium":"social","visits":1} | ||
| {"utm_medium":"display","visits":1} | ||
| {"utm_medium":"email","visits":1} | ||
|
|
||
| - name: Filtered by pathname - /about/ | ||
| description: Filtered by pathname - /about/ | ||
| parameters: site_uuid=mock_site_uuid&date_from=2100-01-01&date_to=2100-01-07&timezone=Etc/UTC&pathname=%2Fabout%2F | ||
| expected_result: | | ||
| {"utm_medium":"social","visits":3} | ||
| {"utm_medium":"referral","visits":1} | ||
| {"utm_medium":"display","visits":1} | ||
| {"utm_medium":"email","visits":1} | ||
|
|
||
| - name: Filtered by post_uuid - 06b1b0c9-fb53-4a15-a060-3db3fde7b1fc (/about/) | ||
| description: Filtered by post_uuid - 06b1b0c9-fb53-4a15-a060-3db3fde7b1fc (/about/) | ||
| parameters: site_uuid=mock_site_uuid&date_from=2100-01-01&date_to=2100-01-07&timezone=Etc/UTC&post_uuid=06b1b0c9-fb53-4a15-a060-3db3fde7b1fc | ||
| expected_result: | | ||
| {"utm_medium":"social","visits":3} | ||
| {"utm_medium":"referral","visits":1} | ||
| {"utm_medium":"display","visits":1} | ||
| {"utm_medium":"email","visits":1} | ||
|
|
||
| - name: Filtered by source - bing.com | ||
| description: Filtered by source - bing.com | ||
| parameters: site_uuid=mock_site_uuid&date_from=2100-01-01&date_to=2100-01-07&timezone=Etc/UTC&source=bing.com | ||
| expected_result: | | ||
| {"utm_medium":"social","visits":1} | ||
| {"utm_medium":"email","visits":1} | ||
|
|
||
| - name: Filtered by member status - paid | ||
| description: Filtered by member status - paid | ||
| parameters: site_uuid=mock_site_uuid&date_from=2100-01-01&date_to=2100-01-07&timezone=Etc/UTC&member_status=paid | ||
| expected_result: | | ||
| {"utm_medium":"social","visits":2} | ||
| {"utm_medium":"organic","visits":1} | ||
|
|
||
| - name: Filtered by member status - undefined | ||
| description: Filtered by member status - undefined | ||
| parameters: site_uuid=mock_site_uuid&date_from=2100-01-01&date_to=2100-01-07&timezone=Etc/UTC&member_status=undefined | ||
| expected_result: | | ||
| {"utm_medium":"cpc","visits":2} | ||
| {"utm_medium":"referral","visits":1} | ||
| {"utm_medium":"social","visits":1} | ||
|
|
||
| - name: Filtered by timezone - America/Los_Angeles | ||
| description: Filtered by timezone - America/Los_Angeles | ||
| parameters: site_uuid=mock_site_uuid&date_from=2100-01-01&date_to=2100-01-07&timezone=America/Los_Angeles | ||
| expected_result: | | ||
| {"utm_medium":"social","visits":4} | ||
| {"utm_medium":"cpc","visits":1} | ||
| {"utm_medium":"organic","visits":1} | ||
| {"utm_medium":"referral","visits":1} | ||
| {"utm_medium":"display","visits":1} | ||
|
|
||
| - name: Test with multiple filters combined | ||
| description: Test with multiple filters combined | ||
| parameters: site_uuid=mock_site_uuid&date_from=2100-01-01&date_to=2100-01-07&timezone=Etc/UTC&source=bing.com&pathname=%2Fabout%2F | ||
| expected_result: | | ||
| {"utm_medium":"social","visits":1} | ||
| {"utm_medium":"email","visits":1} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the pipe file for api_top_utm_mediums_v2
fd 'api_top_utm_mediums_v2\.pipe$' -aRepository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 177
🏁 Script executed:
#!/bin/bash
# Once we find the file, inspect its content for ORDER BY
find . -name "*api_top_utm_mediums_v2*" -type f | head -20Repository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 233
🏁 Script executed:
cat ./ghost/core/core/server/data/tinybird/endpoints/api_top_utm_mediums_v2.pipeRepository: qodo-benchmark/Ghost-combined-coderabbit
Length of output: 772
Stabilize ordering for tied visit counts in expected results.
The endpoint's ORDER BY clause (order by visits desc) has no secondary sort key. Multiple expected_result blocks contain tied values—utm_mediums with identical visits counts (e.g., several entries with visits=1 in "Date range", "Filtered by location - UK", "Filtered by pathname", etc.). Without a deterministic secondary ordering, these fixtures will produce flaky tests. Add a secondary ORDER BY (e.g., utm_medium ASC) to the endpoint to ensure consistent result order.
🤖 Prompt for AI Agents
In `@ghost/core/core/server/data/tinybird/tests/api_top_utm_mediums_v2.yaml`
around lines 2 - 79, The SQL ordering is non-deterministic because the endpoint
uses "order by visits desc" with tied visit counts; update the endpoint's ORDER
BY clause to add a deterministic secondary key so results sort consistently
(e.g., change "order by visits desc" to "ORDER BY visits DESC, utm_medium ASC")
wherever the query is defined (the ORDER BY clause / "order by visits desc" in
the Tinybird endpoint or query builder), and ensure any code that constructs
ordering uses the same deterministic ordering so the YAML fixtures no longer
flake.
2 participants
Benchmark PR from agentic-review-benchmarks#13
Summary by CodeRabbit
Release Notes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.