Open
Conversation
📝 WalkthroughWalkthroughThe changes extend analytics filtering across cloud and community backends by introducing a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/apps/cloud/src/analytics/analytics.service.ts`:
- Around line 4954-4998: The analytics arm inside allProfileDataCTE is missing
${filtersQuery}, causing pageviews/sessions to be unfiltered while events are
filtered; update the analytics SELECT (the first UNION ALL branch in
allProfileDataCTE) to include the same ${filtersQuery} (and keep
${profileTypeFilter} and pid checks) so that analytics rows are restricted to
the same event-level filters used for customEV, ensuring eventsCount and
pageview/session totals represent the same filtered slice.
- Around line 4954-5035: The all_profile_data CTE currently omits the requested
time window, so update the CTE construction (both branches where
customEVFilterApplied is true/false and inside the analytics SELECT, the
customEV SELECT, and the IN subquery) to constrain rows to the groupFrom/groupTo
range by adding the created time filter (use groupFrom/groupTo parameters into
the ClickHouse query placeholders, e.g., created >= {groupFrom} AND created <=
{groupTo} or an equivalent BETWEEN expression). Ensure you add this filter
alongside existing WHERE clauses in the code that builds allProfileDataCTE
(references: allProfileDataCTE variable, groupFrom, groupTo,
customEVFilterApplied, filtersQuery, profileTypeFilter) so
sessionsCount/pageviewsCount/eventsCount/firstSeen/lastSeen reflect the selected
period.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2e5369c8-9efd-46b5-8066-30fc93d5fc37
📒 Files selected for processing (4)
backend/apps/cloud/src/analytics/analytics.controller.tsbackend/apps/cloud/src/analytics/analytics.service.tsbackend/apps/community/src/analytics/analytics.controller.tsbackend/apps/community/src/analytics/analytics.service.ts
Comment on lines
+4954
to
+4998
| if (customEVFilterApplied) { | ||
| allProfileDataCTE = ` | ||
| all_profile_data AS ( | ||
| SELECT | ||
| profileId, | ||
| psid, | ||
| cc, | ||
| os, | ||
| br, | ||
| dv, | ||
| created, | ||
| 1 AS isPageview, | ||
| 0 AS isEvent | ||
| FROM analytics | ||
| WHERE pid = {pid:FixedString(12)} | ||
| AND profileId IS NOT NULL | ||
| AND profileId != '' | ||
| ${profileTypeFilter} | ||
| AND profileId IN ( | ||
| SELECT DISTINCT profileId | ||
| FROM customEV | ||
| WHERE pid = {pid:FixedString(12)} | ||
| AND profileId IS NOT NULL | ||
| AND profileId != '' | ||
| ${profileTypeFilter} | ||
| ${filtersQuery} | ||
| ) | ||
| UNION ALL | ||
| SELECT | ||
| profileId, | ||
| psid, | ||
| cc, | ||
| os, | ||
| br, | ||
| dv, | ||
| created, | ||
| 0 AS isPageview, | ||
| 1 AS isEvent | ||
| FROM customEV | ||
| WHERE pid = {pid:FixedString(12)} | ||
| AND profileId IS NOT NULL | ||
| AND profileId != '' | ||
| ${profileTypeFilter} | ||
| ${filtersQuery} | ||
| )` |
There was a problem hiding this comment.
Avoid mixing filtered event totals with unfiltered analytics totals.
In this branch, ${filtersQuery} is applied to the customEV arm but not to the analytics arm. Once a profile matches one filtered event, the row starts accumulating unfiltered pageviews/sessions while eventsCount stays filtered, so the profile totals no longer describe the same slice of data.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/apps/cloud/src/analytics/analytics.service.ts` around lines 4954 -
4998, The analytics arm inside allProfileDataCTE is missing ${filtersQuery},
causing pageviews/sessions to be unfiltered while events are filtered; update
the analytics SELECT (the first UNION ALL branch in allProfileDataCTE) to
include the same ${filtersQuery} (and keep ${profileTypeFilter} and pid checks)
so that analytics rows are restricted to the same event-level filters used for
customEV, ensuring eventsCount and pageview/session totals represent the same
filtered slice.
Comment on lines
+4954
to
+5035
| if (customEVFilterApplied) { | ||
| allProfileDataCTE = ` | ||
| all_profile_data AS ( | ||
| SELECT | ||
| profileId, | ||
| psid, | ||
| cc, | ||
| os, | ||
| br, | ||
| dv, | ||
| created, | ||
| 1 AS isPageview, | ||
| 0 AS isEvent | ||
| FROM analytics | ||
| WHERE pid = {pid:FixedString(12)} | ||
| AND profileId IS NOT NULL | ||
| AND profileId != '' | ||
| ${profileTypeFilter} | ||
| AND profileId IN ( | ||
| SELECT DISTINCT profileId | ||
| FROM customEV | ||
| WHERE pid = {pid:FixedString(12)} | ||
| AND profileId IS NOT NULL | ||
| AND profileId != '' | ||
| ${profileTypeFilter} | ||
| ${filtersQuery} | ||
| ) | ||
| UNION ALL | ||
| SELECT | ||
| profileId, | ||
| psid, | ||
| cc, | ||
| os, | ||
| br, | ||
| dv, | ||
| created, | ||
| 0 AS isPageview, | ||
| 1 AS isEvent | ||
| FROM customEV | ||
| WHERE pid = {pid:FixedString(12)} | ||
| AND profileId IS NOT NULL | ||
| AND profileId != '' | ||
| ${profileTypeFilter} | ||
| ${filtersQuery} | ||
| )` | ||
| } else { | ||
| allProfileDataCTE = ` | ||
| all_profile_data AS ( | ||
| SELECT | ||
| profileId, | ||
| psid, | ||
| cc, | ||
| os, | ||
| br, | ||
| dv, | ||
| created, | ||
| 1 AS isPageview, | ||
| 0 AS isEvent | ||
| FROM analytics | ||
| WHERE pid = {pid:FixedString(12)} | ||
| AND profileId IS NOT NULL | ||
| AND profileId != '' | ||
| ${profileTypeFilter} | ||
| ${filtersQuery} | ||
| UNION ALL | ||
| SELECT | ||
| profileId, | ||
| psid, | ||
| cc, | ||
| os, | ||
| br, | ||
| dv, | ||
| created, | ||
| 0 AS isPageview, | ||
| 1 AS isEvent | ||
| FROM customEV | ||
| WHERE pid = {pid:FixedString(12)} | ||
| AND profileId IS NOT NULL | ||
| AND profileId != '' | ||
| ${profileTypeFilter} | ||
| ${filtersQuery} | ||
| )` |
There was a problem hiding this comment.
Constrain all_profile_data to the requested time window.
groupFrom/groupTo are already passed into this method, but none of the new CTE branches use them. That makes /analytics/profiles aggregate all-time rows, so sessionsCount, pageviewsCount, eventsCount, firstSeen, and lastSeen ignore the selected period.
🐛 Proposed fix
all_profile_data AS (
SELECT
profileId,
@@
FROM analytics
WHERE pid = {pid:FixedString(12)}
AND profileId IS NOT NULL
AND profileId != ''
${profileTypeFilter}
+ AND created BETWEEN {groupFrom:String} AND {groupTo:String}
AND profileId IN (
SELECT DISTINCT profileId
FROM customEV
WHERE pid = {pid:FixedString(12)}
AND profileId IS NOT NULL
AND profileId != ''
${profileTypeFilter}
+ AND created BETWEEN {groupFrom:String} AND {groupTo:String}
${filtersQuery}
)
@@
FROM customEV
WHERE pid = {pid:FixedString(12)}
AND profileId IS NOT NULL
AND profileId != ''
${profileTypeFilter}
+ AND created BETWEEN {groupFrom:String} AND {groupTo:String}
${filtersQuery}
)`
@@
all_profile_data AS (
SELECT
profileId,
@@
FROM analytics
WHERE pid = {pid:FixedString(12)}
AND profileId IS NOT NULL
AND profileId != ''
${profileTypeFilter}
+ AND created BETWEEN {groupFrom:String} AND {groupTo:String}
${filtersQuery}
@@
FROM customEV
WHERE pid = {pid:FixedString(12)}
AND profileId IS NOT NULL
AND profileId != ''
${profileTypeFilter}
+ AND created BETWEEN {groupFrom:String} AND {groupTo:String}
${filtersQuery}
)`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/apps/cloud/src/analytics/analytics.service.ts` around lines 4954 -
5035, The all_profile_data CTE currently omits the requested time window, so
update the CTE construction (both branches where customEVFilterApplied is
true/false and inside the analytics SELECT, the customEV SELECT, and the IN
subquery) to constrain rows to the groupFrom/groupTo range by adding the created
time filter (use groupFrom/groupTo parameters into the ClickHouse query
placeholders, e.g., created >= {groupFrom} AND created <= {groupTo} or an
equivalent BETWEEN expression). Ensure you add this filter alongside existing
WHERE clauses in the code that builds allProfileDataCTE (references:
allProfileDataCTE variable, groupFrom, groupTo, customEVFilterApplied,
filtersQuery, profileTypeFilter) so
sessionsCount/pageviewsCount/eventsCount/firstSeen/lastSeen reflect the selected
period.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
If applicable, please describe what changes were made in this pull request.
Community Edition support
Database migrations
Documentation
Summary by CodeRabbit
New Features
Bug Fixes