feat: Add filters support for Goals & Funnels, goal conditions, and conversion insights#538
Merged
Merged
Conversation
…onversion insights
|
Caution Review failedFailed to post review comments 📝 WalkthroughWalkthroughAdds analytics filters support and multi-condition goals end-to-end: DTOs/interfaces updated; analytics services/controllers compute session-scoped funnels with multi-dimension breakdowns and time-to-convert; goal entities, DTOs, controllers and services persist and apply conditions; frontend API/types, UI (filters, condition editor, sessions drawer) and routes updated; migrations and docs added. ChangesFull PR checkpoint
Estimated code review effort: Possibly related PRs:
Poem:
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/app/pages/Project/View/components/FilterRowsEditor.tsx (1)
416-418:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReconcile invalid operators when hydrating initial filters.
Line 416 loads rows from
initialFilterswithout validating operator compatibility for the selected column. If persisted filters contain an operator now disallowed for that column (e.g., restricted metadata-key columns), the operator dropdown can render in an invalid state until the column is manually changed.🔧 Proposed fix
useEffect(() => { if (!active) { return } - const nextRows = filtersToRows(initialFilters) + const nextRows = filtersToRows(initialFilters).map((row) => { + const operatorOptions = getOperatorsForColumn(row.column) + const operator = operatorOptions.some( + (option) => option.value === row.operator, + ) + ? row.operator + : 'is' + return { ...row, operator } + }) setFilterRows(nextRows.length ? nextRows : [createEmptyFilterRow()]) }, [active, initialFilters, resetKey])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/app/pages/Project/View/components/FilterRowsEditor.tsx` around lines 416 - 418, When hydrating initialFilters into rows the code path (filtersToRows followed by setFilterRows) does not validate that each row.operator is allowed for its selected column, causing the operator dropdown to show invalid values; update filtersToRows (or add a reconciliation step before calling setFilterRows) to, for every row produced from initialFilters, compute the allowed operator list for that row's column (use the same logic that populates the operator dropdown), and if row.operator is not in that allowed list replace it with a safe fallback (e.g., the first allowed operator or a canonical default like "equals") or clear it; ensure this fix touches filtersToRows, createEmptyFilterRow usage, and the hydration block that calls setFilterRows so hydrated rows always have compatible operators.
🧹 Nitpick comments (4)
backend/apps/community/src/goal/goal.controller.ts (2)
635-641: 💤 Low valueSame SQL interpolation concern as cloud controller.
Direct interpolation of
PROFILE_PREFIX_USERconstant in SQL string.♻️ Suggested refactor using parameter
- SELECT 'profileTypes' AS kind, if(startsWith(profileId, '${AnalyticsService.PROFILE_PREFIX_USER}'), 'identified', 'anonymous') AS val, count() AS cnt + SELECT 'profileTypes' AS kind, if(startsWith(profileId, {profilePrefixUser:String}), 'identified', 'anonymous') AS val, count() AS cntThen add
profilePrefixUser: AnalyticsService.PROFILE_PREFIX_USERto the query params.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/apps/community/src/goal/goal.controller.ts` around lines 635 - 641, The SQL currently interpolates AnalyticsService.PROFILE_PREFIX_USER directly into the query string (in the UNION SELECT using startsWith(profileId, '${AnalyticsService.PROFILE_PREFIX_USER}')), which risks injection and makes tests harder; change the SQL to use a parameter placeholder for the profile prefix (e.g. startsWith(profileId, {profilePrefixUser}) or the query-param syntax your DB client uses) and add profilePrefixUser: AnalyticsService.PROFILE_PREFIX_USER to the query parameters passed to the method that executes the query (refer to the query string and the call site around the UNION SELECT in goal.controller.ts and the AnalyticsService.PROFILE_PREFIX_USER symbol).
361-554: ⚖️ Poor tradeoffConsider extracting shared condition-building logic.
The condition expression building, goal event condition, and conversions subquery logic is duplicated between cloud and community controllers (~200 lines). Consider extracting this into a shared utility or base class to reduce maintenance burden.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/apps/community/src/goal/goal.controller.ts` around lines 361 - 554, The controller duplicates ~200 lines of condition and query building across buildConditionExpression, buildGoalEventCondition, and buildGoalConversionsSubquery; extract this logic into a shared module or base class (e.g., GoalConditionBuilder with methods buildConditionExpression(condition,index), buildGoalEventCondition(goal), buildGoalConversionsSubquery(goal,filtersQuery)) and replace the duplicated implementations in both cloud and community controllers by delegating to that single implementation; ensure the shared helper returns the same shapes ({condition, params} and {query, params}), preserve use of GOAL_CONDITION_COLUMNS and GoalType/Goal interfaces by importing them into the new module, and update callers to merge params exactly as existing code does.backend/apps/cloud/src/goal/goal.controller.ts (1)
715-721: 💤 Low valueAvoid direct string interpolation in SQL queries.
The
PROFILE_PREFIX_USERconstant is directly interpolated into the SQL string. While this is a static constant and currently safe, it's better practice to use parameterized queries consistently to prevent future refactoring from inadvertently introducing SQL injection risks.♻️ Suggested refactor using parameter
- SELECT 'profileTypes' AS kind, if(startsWith(profileId, '${AnalyticsService.PROFILE_PREFIX_USER}'), 'identified', 'anonymous') AS val, count() AS cnt + SELECT 'profileTypes' AS kind, if(startsWith(profileId, {profilePrefixUser:String}), 'identified', 'anonymous') AS val, count() AS cntThen add
profilePrefixUser: AnalyticsService.PROFILE_PREFIX_USERto the query params.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/apps/cloud/src/goal/goal.controller.ts` around lines 715 - 721, The SQL currently interpolates AnalyticsService.PROFILE_PREFIX_USER directly into the query string (in the SELECT branch querying session_info using startsWith(profileId, '...')), which risks future injection; change the SQL to use a parameter placeholder instead of string interpolation for the profile prefix in the startsWith call, and add a corresponding param (e.g. profilePrefixUser: AnalyticsService.PROFILE_PREFIX_USER) to the query parameters passed where the query is executed so the startsWith(profileId, <param>) is provided safely; ensure you update the query invocation in goal.controller.ts to include the new profilePrefixUser param.backend/apps/community/src/analytics/analytics.service.ts (1)
1761-1777: ⚡ Quick winRestrict
session_infoto the funnel cohort.This CTE currently aggregates every session in the requested time range and only throws most of them away in the later join with
expanded. On busy projects that makes breakdown requests scale with total traffic, not with the actual funnel cohort. Filteringsession_infobyfunnel_sessionshere would keep the query much cheaper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/apps/community/src/analytics/analytics.service.ts` around lines 1761 - 1777, The session_info CTE is aggregating all sessions in the time range and should be restricted to the funnel cohort to avoid scaling with total traffic; modify the session_info definition in analytics.service.ts so it only includes sessions present in the funnel cohort (e.g., add a WHERE psid IN (SELECT psid FROM funnel_sessions) or INNER JOIN funnel_sessions USING (psid) before the GROUP BY) so subsequent joins (like with expanded) operate only on funnel_sessions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/apps/cloud/src/analytics/analytics.service.ts`:
- Around line 1908-1911: The code assigns untrusted keys into a plain object
(details[type][step][row.val] = row.cnt), which allows prototype pollution if
row.val is "__proto__", "constructor", etc.; change the implementation so that
the bucket objects are created with a null prototype (e.g., use
Object.create(null) whenever initializing details[type] or details[type][step])
or validate/whitelist row.val before assignment and skip or normalize reserved
keys; locate the creation/initialization of details[type] and
details[type][step] and replace new plain {} with null-prototype objects or add
a guard that rejects keys like "__proto__", "constructor", "prototype" prior to
writing.
In `@backend/apps/community/src/analytics/analytics.service.ts`:
- Around line 2083-2132: The funnel timestamps are taken from global minIf/maxIf
and can mismatch the actual matched funnel run; update funnel_sessions to
extract the matched-run timestamps (firstStepAt and conversionAt) from the same
ordered event sequence used by windowFunnel (e.g. compute an ordered array of
events for each psid, use the windowFunnel result to pick the specific
indices/timestamps for the matching run instead of minIf/maxIf) so firstStepAt
and conversionAt pair correctly; also change session_starts (and first_pages
lookup) to derive sessionStart and firstPageAt scoped to the visit that contains
the matched funnel run (filter sessions/events by the matched-run time window
per psid) rather than using min(firstSeen) across all history. Ensure you update
references in funnel_sessions, session_starts and first_pages to use the
matched-run boundaries.
In `@web/app/routes/projects`.$id.tsx:
- Around line 700-705: The code unguardedly calls JSON.parse on form values
(metadataFilters and conditions, and the other parsing at lines ~739-744) which
will throw on invalid input; wrap each JSON.parse in a try/catch or use a small
safeParse helper (e.g., safeJsonParse(value, default) or try { parsed =
JSON.parse(...) } catch { return new Response('Invalid JSON in <field>', {
status: 400 }) }) to return a controlled 400 instead of letting the action throw
a 500, and also validate the parsed type (ensure metadataFilters is an array,
conditions is the expected object/null) before continuing; update the parsing
sites that reference metadataFilters and conditions to use this guarded parsing.
---
Outside diff comments:
In `@web/app/pages/Project/View/components/FilterRowsEditor.tsx`:
- Around line 416-418: When hydrating initialFilters into rows the code path
(filtersToRows followed by setFilterRows) does not validate that each
row.operator is allowed for its selected column, causing the operator dropdown
to show invalid values; update filtersToRows (or add a reconciliation step
before calling setFilterRows) to, for every row produced from initialFilters,
compute the allowed operator list for that row's column (use the same logic that
populates the operator dropdown), and if row.operator is not in that allowed
list replace it with a safe fallback (e.g., the first allowed operator or a
canonical default like "equals") or clear it; ensure this fix touches
filtersToRows, createEmptyFilterRow usage, and the hydration block that calls
setFilterRows so hydrated rows always have compatible operators.
---
Nitpick comments:
In `@backend/apps/cloud/src/goal/goal.controller.ts`:
- Around line 715-721: The SQL currently interpolates
AnalyticsService.PROFILE_PREFIX_USER directly into the query string (in the
SELECT branch querying session_info using startsWith(profileId, '...')), which
risks future injection; change the SQL to use a parameter placeholder instead of
string interpolation for the profile prefix in the startsWith call, and add a
corresponding param (e.g. profilePrefixUser:
AnalyticsService.PROFILE_PREFIX_USER) to the query parameters passed where the
query is executed so the startsWith(profileId, <param>) is provided safely;
ensure you update the query invocation in goal.controller.ts to include the new
profilePrefixUser param.
In `@backend/apps/community/src/analytics/analytics.service.ts`:
- Around line 1761-1777: The session_info CTE is aggregating all sessions in the
time range and should be restricted to the funnel cohort to avoid scaling with
total traffic; modify the session_info definition in analytics.service.ts so it
only includes sessions present in the funnel cohort (e.g., add a WHERE psid IN
(SELECT psid FROM funnel_sessions) or INNER JOIN funnel_sessions USING (psid)
before the GROUP BY) so subsequent joins (like with expanded) operate only on
funnel_sessions.
In `@backend/apps/community/src/goal/goal.controller.ts`:
- Around line 635-641: The SQL currently interpolates
AnalyticsService.PROFILE_PREFIX_USER directly into the query string (in the
UNION SELECT using startsWith(profileId,
'${AnalyticsService.PROFILE_PREFIX_USER}')), which risks injection and makes
tests harder; change the SQL to use a parameter placeholder for the profile
prefix (e.g. startsWith(profileId, {profilePrefixUser}) or the query-param
syntax your DB client uses) and add profilePrefixUser:
AnalyticsService.PROFILE_PREFIX_USER to the query parameters passed to the
method that executes the query (refer to the query string and the call site
around the UNION SELECT in goal.controller.ts and the
AnalyticsService.PROFILE_PREFIX_USER symbol).
- Around line 361-554: The controller duplicates ~200 lines of condition and
query building across buildConditionExpression, buildGoalEventCondition, and
buildGoalConversionsSubquery; extract this logic into a shared module or base
class (e.g., GoalConditionBuilder with methods
buildConditionExpression(condition,index), buildGoalEventCondition(goal),
buildGoalConversionsSubquery(goal,filtersQuery)) and replace the duplicated
implementations in both cloud and community controllers by delegating to that
single implementation; ensure the shared helper returns the same shapes
({condition, params} and {query, params}), preserve use of
GOAL_CONDITION_COLUMNS and GoalType/Goal interfaces by importing them into the
new module, and update callers to merge params exactly as existing code does.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ad88676b-d73d-4401-96ca-04693c9635aa
📒 Files selected for processing (37)
backend/apps/cloud/src/analytics/analytics.controller.tsbackend/apps/cloud/src/analytics/analytics.service.tsbackend/apps/cloud/src/analytics/dto/get-funnel-sessions.dto.tsbackend/apps/cloud/src/analytics/dto/getFunnels.dto.tsbackend/apps/cloud/src/analytics/interfaces/index.tsbackend/apps/cloud/src/goal/dto/goal.dto.tsbackend/apps/cloud/src/goal/entity/goal.entity.tsbackend/apps/cloud/src/goal/goal.controller.tsbackend/apps/community/src/analytics/analytics.controller.tsbackend/apps/community/src/analytics/analytics.service.tsbackend/apps/community/src/analytics/dto/get-funnel-sessions.dto.tsbackend/apps/community/src/analytics/dto/getFunnels.dto.tsbackend/apps/community/src/analytics/interfaces/index.tsbackend/apps/community/src/goal/dto/goal.dto.tsbackend/apps/community/src/goal/entity/goal.entity.tsbackend/apps/community/src/goal/goal.controller.tsbackend/apps/community/src/goal/goal.service.tsbackend/migrations/clickhouse/initialise_selfhosted.jsbackend/migrations/clickhouse/selfhosted_2025_12_07_goals.jsbackend/migrations/clickhouse/selfhosted_2026_05_20_goal_conditions.jsbackend/migrations/mysql/2026_05_20_goal_conditions.sqldocs/content/docs/analytics-dashboard/funnels.mdxdocs/content/docs/analytics-dashboard/goals.mdxdocs/content/docs/api/stats.mdxweb/app/api/api.server.tsweb/app/lib/models/Project.tsweb/app/pages/Project/View/ViewProject.tsxweb/app/pages/Project/View/components/FilterRowsEditor.tsxweb/app/pages/Project/View/utils/projectViewSegments.tsweb/app/pages/Project/tabs/Funnels/FunnelsView.tsxweb/app/pages/Project/tabs/Goals/GoalSettingsModal.tsxweb/app/pages/Project/tabs/Goals/GoalsView.tsxweb/app/pages/Project/tabs/Traffic/SessionsDrawer.tsxweb/app/routes/api.analytics.tsweb/app/routes/projects.$id.tsxweb/app/ui/FilterValueInput.tsxweb/public/locales/en.json
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
backend/apps/community/src/goal/goal.controller.ts (1)
829-837:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winExclude
psid = 0from the total-session count here as well.The numerator only considers real sessions (
psid != 0inside the conversions subquery), but the denominator currently does not. That makesconversionRatetoo low whenever unattributed events are present.🛠️ Minimal fix
const totalSessionsQuery = ` SELECT uniqExact(psid) as totalSessions FROM events WHERE pid = {pid:FixedString(12)} AND type IN ('pageview', 'custom_event') + AND psid != 0 ${filtersQuery} AND created BETWEEN {groupFrom:String} AND {groupTo:String} `🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/apps/community/src/goal/goal.controller.ts` around lines 829 - 837, The totalSessionsQuery currently counts uniqExact(psid) but doesn't exclude unattributed sessions (psid = 0), causing the denominator to mismatch the conversions subquery; update the WHERE clause in the totalSessionsQuery (the variable named totalSessionsQuery in goal.controller.ts) to add a condition excluding psid = 0 (e.g., AND psid != 0) so the total session count matches the conversions logic that already filters psid != 0.backend/apps/cloud/src/goal/goal.controller.ts (1)
911-919:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep the conversion-rate denominator aligned with the conversions subquery.
The conversions CTE filters out
psid = 0, buttotalSessionsQuerystill counts it. That understatesconversionRatewhenever unattributed events exist, because the denominator can include one extra pseudo-session that can never convert.🛠️ Minimal fix
const totalSessionsQuery = ` SELECT uniqExact(psid) as totalSessions FROM events WHERE pid = {pid:FixedString(12)} AND type IN ('pageview', 'custom_event') + AND psid != 0 ${filtersQuery} AND created BETWEEN {groupFrom:String} AND {groupTo:String} `🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/apps/cloud/src/goal/goal.controller.ts` around lines 911 - 919, The totalSessionsQuery currently counts all psid values but the conversions CTE excludes psid = 0, causing a mismatched denominator; update the totalSessionsQuery (the template string named totalSessionsQuery) to apply the same psid filter as conversions (e.g., add "AND psid != 0" or the same condition used in the conversions CTE) so the denominator and conversions subquery are aligned when computing conversionRate.web/app/pages/Project/View/components/FilterRowsEditor.tsx (1)
355-362:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon’t disable suggestions for static
ev:value/tag:valuecolumns.The new fast-path treats every column containing
:as non-fetchable. That catchesev:valueandtag:valuetoo, so those valid columns now get an empty cache and never callfetchFilters().🛠️ Suggested fix
async (column: string) => { - if (getMetadataColumnBase(column) !== column || column.includes(':')) { + if (getMetadataColumnBase(column) !== column) { setFilterValuesCache((prev) => prev[column] ? prev : { ...prev, [column]: [] }, ) return }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/app/pages/Project/View/components/FilterRowsEditor.tsx` around lines 355 - 362, The fast-path in fetchFilterValues currently treats any column with ':' as non-fetchable and sets an empty cache; update the condition in fetchFilterValues (and related logic that uses getMetadataColumnBase and setFilterValuesCache) so that columns with ':' are only skipped when they are not metadata base OR when they are namespaced columns that we explicitly want to ignore — allow known static namespaced columns like those starting with "ev:" or "tag:" to proceed to fetchFilters() instead of short-circuiting; change the if so it checks getMetadataColumnBase(column) !== column && !(column.startsWith('ev:') || column.startsWith('tag:')) (or equivalent whitelist) so ev:value and tag:value run the normal fetch path and populate the cache.
♻️ Duplicate comments (1)
backend/apps/community/src/analytics/analytics.service.ts (1)
2152-2174:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
sessionStartandfirstPageAtare still anchored to the historicalpsid, not the matched funnel visit.
session_startsstill doesmin(s.firstSeen)fromsessions FINAL, so a reusedpsidkeeps pulling in the oldest visit that ever used that identifier. That makesfromSessionStartandfromFirstPagedrift upward again for later conversions, becausefirst_pagesscans from that inflated start instead of the visit that actually producedconversionAt. Please derive the visit boundary from the matched funnel run itself, or from a true per-visit session record, before calculating these two metrics.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/apps/community/src/analytics/analytics.service.ts` around lines 2152 - 2174, session_starts and first_pages currently compute sessionStart as min(s.firstSeen) over all sessions for a psid, which pulls the oldest visit; instead, identify the specific session that matches the funnel run and use that session's boundaries. Change the session_starts CTE to join sessions FINAL s to funnel_sessions fs with the predicate s.firstSeen <= fs.firstStepAt AND s.lastSeen >= fs.conversionAt and select s.firstSeen (not min(...)) as sessionStart (or otherwise use a per-run visit start column from fs if present), and update first_pages to use that sessionStart (or s.firstSeen constrained to the matched session) when computing firstPageAt; reference CTEs session_starts, first_pages and tables/aliases funnel_sessions fs and sessions FINAL s, and fields sessionStart, firstPageAt, fs.firstStepAt and fs.conversionAt.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/app/pages/Project/View/components/FilterRowsEditor.tsx`:
- Around line 78-97: The operator-restriction logic only matches exact column
names so dynamic metadata keys like "ev:key:utm_campaign" bypass rules; update
getOperatorsForColumn to detect metadata-key columns by prefix (e.g.,
column.startsWith('ev:key') || column.startsWith('tag:key')) instead of strict
equality, so it returns the restricted OPERATORS for any dynamic metadata-key
column, and keep normalizeFilterOperator using getOperatorsForColumn so
unsupported preloaded operators get replaced with the first allowed option.
In `@web/app/routes/projects`.$id.tsx:
- Around line 165-216: isGoalConditions currently only validates shape and
allows cases the backend converts to impossible queries; update isGoalConditions
(used by parseGoalJsonFields) to reject unsupported/ambiguous cases: enforce
that condition.field is one of the allowed field names for the given
condition.eventType (use GOAL_CONDITION_EVENT_TYPES to map/validate allowed
fields), require condition.metadataKey to be a non-empty string whenever
relation/ eventType implies a metadata condition (or when condition.field
indicates metadata access), and for operators in GOAL_CONDITION_OPERATORS that
require a value, reject condition.value if it's undefined or an empty string;
return false for any of these invalid cases so parseGoalJsonFields returns an
error instead of accepting shapes the backend would turn into 1=0.
---
Outside diff comments:
In `@backend/apps/cloud/src/goal/goal.controller.ts`:
- Around line 911-919: The totalSessionsQuery currently counts all psid values
but the conversions CTE excludes psid = 0, causing a mismatched denominator;
update the totalSessionsQuery (the template string named totalSessionsQuery) to
apply the same psid filter as conversions (e.g., add "AND psid != 0" or the same
condition used in the conversions CTE) so the denominator and conversions
subquery are aligned when computing conversionRate.
In `@backend/apps/community/src/goal/goal.controller.ts`:
- Around line 829-837: The totalSessionsQuery currently counts uniqExact(psid)
but doesn't exclude unattributed sessions (psid = 0), causing the denominator to
mismatch the conversions subquery; update the WHERE clause in the
totalSessionsQuery (the variable named totalSessionsQuery in goal.controller.ts)
to add a condition excluding psid = 0 (e.g., AND psid != 0) so the total session
count matches the conversions logic that already filters psid != 0.
In `@web/app/pages/Project/View/components/FilterRowsEditor.tsx`:
- Around line 355-362: The fast-path in fetchFilterValues currently treats any
column with ':' as non-fetchable and sets an empty cache; update the condition
in fetchFilterValues (and related logic that uses getMetadataColumnBase and
setFilterValuesCache) so that columns with ':' are only skipped when they are
not metadata base OR when they are namespaced columns that we explicitly want to
ignore — allow known static namespaced columns like those starting with "ev:" or
"tag:" to proceed to fetchFilters() instead of short-circuiting; change the if
so it checks getMetadataColumnBase(column) !== column &&
!(column.startsWith('ev:') || column.startsWith('tag:')) (or equivalent
whitelist) so ev:value and tag:value run the normal fetch path and populate the
cache.
---
Duplicate comments:
In `@backend/apps/community/src/analytics/analytics.service.ts`:
- Around line 2152-2174: session_starts and first_pages currently compute
sessionStart as min(s.firstSeen) over all sessions for a psid, which pulls the
oldest visit; instead, identify the specific session that matches the funnel run
and use that session's boundaries. Change the session_starts CTE to join
sessions FINAL s to funnel_sessions fs with the predicate s.firstSeen <=
fs.firstStepAt AND s.lastSeen >= fs.conversionAt and select s.firstSeen (not
min(...)) as sessionStart (or otherwise use a per-run visit start column from fs
if present), and update first_pages to use that sessionStart (or s.firstSeen
constrained to the matched session) when computing firstPageAt; reference CTEs
session_starts, first_pages and tables/aliases funnel_sessions fs and sessions
FINAL s, and fields sessionStart, firstPageAt, fs.firstStepAt and
fs.conversionAt.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f555ab4f-a48e-4bf3-8af5-68c73b15efc2
📒 Files selected for processing (6)
backend/apps/cloud/src/analytics/analytics.service.tsbackend/apps/cloud/src/goal/goal.controller.tsbackend/apps/community/src/analytics/analytics.service.tsbackend/apps/community/src/goal/goal.controller.tsweb/app/pages/Project/View/components/FilterRowsEditor.tsxweb/app/routes/projects.$id.tsx
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
Documentation