Enable filtering on pageview properties #800
Conversation
Added filtering for pageview url parameters on goals and funnels
|
@stijnie2210 is attempting to deploy a commit to the goldflag's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughAdds optional multi-property filtering via Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as FunnelForm / GoalFormModal
participant State as UI State (propertyFilters)
participant API as Server (create/update/get)
participant DB as ClickHouse/Postgres
User->>UI: Edit/add/remove filter rows
UI->>State: Update propertyFilters state
User->>UI: Submit form / request
UI->>API: Send payload (includes propertyFilters)
rect rgb(220,235,245)
API->>API: Validate payload (schema accepts propertyFilters)
API->>API: Build filters list (propertyFilters or legacy pair)
API->>API: For each filter -> choose accessor (url_parameters or props)
API->>API: Apply type-aware comparison (string/number/boolean)
API->>DB: Execute generated SQL / query
end
DB-->>API: Results
API-->>UI: Response
UI-->>User: Render results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
client/src/api/analytics/endpoints/goals.ts (1)
52-61:CreateGoalParams.configis missing thepropertyFiltersfield.The
Goalinterface includespropertyFiltersin its config (lines 17-20), butCreateGoalParams.configdoesn't include it. This type mismatch will cause TypeScript errors when attempting to create or update goals with property filters.🔧 Proposed fix
export interface CreateGoalParams { name?: string; goalType: "path" | "event"; config: { pathPattern?: string; eventName?: string; eventPropertyKey?: string; eventPropertyValue?: string | number | boolean; + propertyFilters?: Array<{ + key: string; + value: string | number | boolean; + }>; }; }client/src/app/[site]/goals/components/GoalFormModal.tsx (2)
112-116:useEffectoverwritesusePropertieswithout checkingpropertyFilters.The initial state at lines 73-75 correctly checks both
propertyFiltersand legacy fields. However, thisuseEffectruns on modal open and only checks the legacyeventPropertyKey/eventPropertyValue, ignoringpropertyFilters. This will incorrectly disable property filtering when editing a goal that uses the newpropertyFiltersformat.🔧 Proposed fix
useEffect(() => { if (isOpen) { - setUseProperties(!!goal?.config.eventPropertyKey && !!goal?.config.eventPropertyValue); + const hasFilters = !!(goal?.config.propertyFilters?.length || + (goal?.config.eventPropertyKey && goal?.config.eventPropertyValue !== undefined)); + setUseProperties(hasFilters); } -}, [isOpen, goal?.config.eventPropertyKey, goal?.config.eventPropertyValue]); +}, [isOpen, goal?.config.propertyFilters, goal?.config.eventPropertyKey, goal?.config.eventPropertyValue]);
205-209: ResetpropertyFiltersstate after successful submission.After successful submission,
usePropertiesis reset butpropertyFiltersis not. If the modal is reopened for a new goal, stale filter values may persist sinceuseStateinitializers only run on mount.🔧 Proposed fix
// Reset form and state after successful submission form.reset(); setUseProperties(false); + setPropertyFilters([{ key: "", value: "" }]); setIsOpen(false);server/src/api/analytics/funnels/getFunnel.ts (1)
127-141:SessionActionsCTE does not includeurl_parameterscolumn.The CTE selects specific columns but omits
url_parameters. However, the page step conditions at line 79 referenceurl_parameters[key]. Since the CTE is used as the source for step matching, this may cause query failures when filtering page steps by URL parameters.🔧 Proposed fix
SessionActions AS ( SELECT session_id, timestamp, pathname, event_name, type, props, - hostname + hostname, + url_parameters FROM events
🤖 Fix all issues with AI agents
In @client/src/app/[site]/funnels/components/FunnelForm.tsx:
- Around line 279-287: The onChange handler (and similar value input and remove
button handlers) mutates nested state directly: newStepPropertyFilters is a
shallow copy but newStepPropertyFilters[index][filterIndex].key is modified in
place; instead, create immutable copies for the inner array and object before
updating: copy stepPropertyFilters, copy the specific step array (e.g.,
stepProps = [...newStepPropertyFilters[index]]), copy the specific filter object
(e.g., filterObj = { ...stepProps[filterIndex] }), update filterObj.key or
value, replace stepProps[filterIndex] with the new object, assign back to
newStepPropertyFilters[index] and call
setStepPropertyFilters(newStepPropertyFilters); also update steps immutably
(copy steps to newSteps, copy the specific step object, set its propertyFilters
to the new array, then setSteps(newSteps)); apply the same pattern in the value
input handler and the remove button handler to avoid direct mutation.
🧹 Nitpick comments (2)
client/src/app/[site]/funnels/components/FunnelForm.tsx (1)
326-339: Consider syncingstepswhen adding a new filter row.Adding a filter row updates
stepPropertyFiltersbut notsteps[index].propertyFilters. While empty filters are likely filtered out on submission, this creates temporary inconsistency between the local UI state and the step data.If you want consistent state, also update
stepshere. Otherwise, consider adding a comment explaining the intentional divergence.client/src/app/[site]/goals/components/GoalFormModal.tsx (1)
344-390: Duplicated property filter UI for path and event goals.The filter input UI (key/value inputs, add/remove buttons) is nearly identical between path goals (lines 344-390) and event goals (lines 425-471). Consider extracting a shared
PropertyFiltersEditorcomponent.This would reduce ~50 lines of duplication and make future changes easier to maintain. Based on learnings, this aligns with React functional component best practices.
Also applies to: 425-471
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
client/src/api/analytics/endpoints/funnels.tsclient/src/api/analytics/endpoints/goals.tsclient/src/app/[site]/funnels/components/FunnelForm.tsxclient/src/app/[site]/goals/components/GoalFormModal.tsxserver/src/api/analytics/funnels/getFunnel.tsserver/src/api/analytics/goals/getGoals.tsserver/src/db/postgres/schema.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript with strict typing throughout both client and server
Use camelCase for variables and functions
Use PascalCase for components and types
Group imports by external, then internal (alphabetical within groups)
Error handling: Use try/catch blocks with specific error types
Files:
server/src/db/postgres/schema.tsserver/src/api/analytics/goals/getGoals.tsserver/src/api/analytics/funnels/getFunnel.tsclient/src/api/analytics/endpoints/goals.tsclient/src/app/[site]/funnels/components/FunnelForm.tsxclient/src/api/analytics/endpoints/funnels.tsclient/src/app/[site]/goals/components/GoalFormModal.tsx
client/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Client: React functional components with minimal useEffect and inline functions
Files:
client/src/api/analytics/endpoints/goals.tsclient/src/app/[site]/funnels/components/FunnelForm.tsxclient/src/api/analytics/endpoints/funnels.tsclient/src/app/[site]/goals/components/GoalFormModal.tsx
client/**/*.{css,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Dark mode is default theme
Files:
client/src/app/[site]/funnels/components/FunnelForm.tsxclient/src/app/[site]/goals/components/GoalFormModal.tsx
🧠 Learnings (3)
📚 Learning: 2025-11-25T02:34:24.979Z
Learnt from: CR
Repo: rybbit-io/rybbit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T02:34:24.979Z
Learning: Applies to client/**/*.{ts,tsx} : Client: React functional components with minimal useEffect and inline functions
Applied to files:
client/src/app/[site]/funnels/components/FunnelForm.tsx
📚 Learning: 2025-11-25T02:34:24.979Z
Learnt from: CR
Repo: rybbit-io/rybbit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T02:34:24.979Z
Learning: Frontend: Use Next.js with Turbopack, Tailwind CSS, Shadcn UI, Tanstack Query, Zustand, Luxon, Nivo, and react-hook-form
Applied to files:
client/src/app/[site]/funnels/components/FunnelForm.tsx
📚 Learning: 2025-11-25T02:34:42.365Z
Learnt from: CR
Repo: rybbit-io/rybbit PR: 0
File: docs/CLAUDE.md:0-0
Timestamp: 2025-11-25T02:34:42.365Z
Learning: Applies to docs/**/*.{ts,tsx} : Use React 19 functional components with hooks
Applied to files:
client/src/app/[site]/funnels/components/FunnelForm.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build Client Images (ubuntu-latest, linux/amd64)
- GitHub Check: Build Client Images (ubuntu-24.04-arm, linux/arm64)
🔇 Additional comments (9)
client/src/api/analytics/endpoints/goals.ts (1)
13-20: Good addition ofpropertyFilterswith backward compatibility.The deprecation comments and new field structure are clear. The type definition properly supports multiple key/value filters.
client/src/api/analytics/endpoints/funnels.ts (1)
11-18: LGTM!The
propertyFiltersaddition toFunnelStepis consistent with the goals implementation and properly propagates through all request/response types that useFunnelStep.client/src/app/[site]/funnels/components/FunnelForm.tsx (1)
49-61: Solid initialization with backward compatibility.The state initialization correctly handles both the new
propertyFiltersarray and legacy single-property fields, with a sensible fallback to an empty filter row.client/src/app/[site]/goals/components/GoalFormModal.tsx (1)
71-83: Good initialization handling for backward compatibility.The state initialization correctly handles both new
propertyFiltersand legacy single-property fields, with appropriate fallback defaults.server/src/db/postgres/schema.ts (1)
228-234: LGTM!The schema extension for
propertyFiltersis well-structured with clear deprecation notes for legacy fields. The JSONB column handles the additive change gracefully without requiring a migration.server/src/api/analytics/goals/getGoals.ts (2)
156-172: Path goal property filtering implementation looks correct.The URL parameter access via
url_parameters[key]with proper escaping and string comparison aligns with how ClickHouse Map types work.
196-211: VerifySqlString.escapeIdcompatibility with ClickHouse JSON property access.
SqlString.escapeIdis from the npm "sqlstring" package, a MySQL-focused library that wraps identifiers in backticks. The patternprops.${SqlString.escapeId(filter.key)}producesprops.\key`` syntax. ClickHouse typically uses double quotes for quoted identifiers, and it's unclear whether backtick-escaped identifiers work correctly for JSON property access in ClickHouse queries. This pattern is used consistently across four production analytics files (getGoals.ts, getGoalSessions.ts, getFunnel.ts, getFunnelStepSessions.ts). Confirm that ClickHouse accepts this syntax or implement ClickHouse-specific identifier escaping.server/src/api/analytics/funnels/getFunnel.ts (2)
96-112: SameSqlString.escapeIdconcern applies here.As noted in
getGoals.ts, verify thatSqlString.escapeId(filter.key)produces valid ClickHouse syntax for JSON property access.
68-83: Page step URL parameter filtering implementation is correct.The filter construction logic properly handles both new
propertyFiltersand legacy fields with appropriate escaping.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/src/app/[site]/funnels/components/FunnelForm.tsx (1)
141-158: State sync issue when re-enabling property filtering.When toggling OFF,
steps[index].propertyFiltersis cleared butstepPropertyFilters[index]retains the old values. If the user toggles ON again, the UI displays the old filters whilesteps[index].propertyFiltersremainsundefineduntil the user types—causing a mismatch between what the user sees and what would be saved.🔧 Proposed fix - sync state on toggle
const togglePropertyFiltering = (index: number, enabled: boolean) => { const newUseProperties = [...useProperties]; newUseProperties[index] = enabled; setUseProperties(newUseProperties); // Clear property fields if disabling if (!enabled) { const newSteps = [...steps]; newSteps[index] = { ...newSteps[index], eventPropertyKey: undefined, eventPropertyValue: undefined, propertyFilters: undefined, }; setSteps(newSteps); + + // Reset stepPropertyFilters to default empty filter + const newStepPropertyFilters = [...stepPropertyFilters]; + newStepPropertyFilters[index] = [{ key: "", value: "" }]; + setStepPropertyFilters(newStepPropertyFilters); } };
🧹 Nitpick comments (2)
client/src/app/[site]/funnels/components/FunnelForm.tsx (2)
339-352: Consider syncingstepswhen adding a new filter row.The "Add Another" handler updates only
stepPropertyFilterswithout syncing tosteps[index].propertyFilters. While empty filters likely shouldn't be persisted, this creates a temporary inconsistency with the key/value onChange handlers that immediately sync both states.If this is intentional (only sync non-empty filters), consider adding a brief comment to clarify the design choice.
273-312: Consider adding accessible labels to filter inputs.The inputs rely solely on placeholders for context. Screen reader users would benefit from explicit labels or
aria-labelattributes.♿ Example using aria-label
<Input - placeholder={step.type === "page" ? "e.g., utm_source" : "e.g., plan_type"} + placeholder={step.type === "page" ? "e.g., utm_source" : "e.g., plan_type"} + aria-label={`Filter ${filterIndex + 1} ${step.type === "page" ? "parameter" : "property"} key`} className="border-neutral-300 dark:border-neutral-700" value={filter.key} onChange={e => {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/src/app/[site]/funnels/components/FunnelForm.tsx
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript with strict typing throughout both client and server
Use camelCase for variables and functions
Use PascalCase for components and types
Group imports by external, then internal (alphabetical within groups)
Error handling: Use try/catch blocks with specific error types
Files:
client/src/app/[site]/funnels/components/FunnelForm.tsx
client/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Client: React functional components with minimal useEffect and inline functions
Files:
client/src/app/[site]/funnels/components/FunnelForm.tsx
client/**/*.{css,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Dark mode is default theme
Files:
client/src/app/[site]/funnels/components/FunnelForm.tsx
🧠 Learnings (3)
📚 Learning: 2025-11-25T02:34:24.979Z
Learnt from: CR
Repo: rybbit-io/rybbit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T02:34:24.979Z
Learning: Applies to client/**/*.{ts,tsx} : Client: React functional components with minimal useEffect and inline functions
Applied to files:
client/src/app/[site]/funnels/components/FunnelForm.tsx
📚 Learning: 2025-11-25T02:34:24.979Z
Learnt from: CR
Repo: rybbit-io/rybbit PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T02:34:24.979Z
Learning: Frontend: Use Next.js with Turbopack, Tailwind CSS, Shadcn UI, Tanstack Query, Zustand, Luxon, Nivo, and react-hook-form
Applied to files:
client/src/app/[site]/funnels/components/FunnelForm.tsx
📚 Learning: 2025-11-25T02:34:42.365Z
Learnt from: CR
Repo: rybbit-io/rybbit PR: 0
File: docs/CLAUDE.md:0-0
Timestamp: 2025-11-25T02:34:42.365Z
Learning: Applies to docs/**/*.{ts,tsx} : Use React 19 functional components with hooks
Applied to files:
client/src/app/[site]/funnels/components/FunnelForm.tsx
🧬 Code graph analysis (1)
client/src/app/[site]/funnels/components/FunnelForm.tsx (4)
client/src/components/ui/switch.tsx (1)
Switch(29-29)client/src/components/ui/label.tsx (1)
Label(21-21)client/src/components/ui/input.tsx (1)
Input(44-44)client/src/components/ui/button.tsx (1)
Button(60-60)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build Client Images (ubuntu-latest, linux/amd64)
- GitHub Check: Build Client Images (ubuntu-24.04-arm, linux/arm64)
🔇 Additional comments (4)
client/src/app/[site]/funnels/components/FunnelForm.tsx (4)
49-61: LGTM - State initialization handles both legacy and new filter formats.The lazy initialization correctly converts existing
propertyFiltersor legacyeventPropertyKey/eventPropertyValueto the UI-friendly string format, with appropriate fallback to an empty filter.
102-106: LGTM - Step addition properly initializes all related state arrays.
108-122: LGTM - Step removal properly cleans up all related state arrays.
279-311: LGTM - Immutable state updates are correctly implemented.The onChange handlers now properly create copies at each nesting level before updating, avoiding direct mutation of React state.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @server/src/api/analytics/goals/getGoalSessions.ts:
- Around line 86-97: The loop in getGoalSessions builds property filters using
propValueAccessor = `props.${SqlString.escapeId(filter.key)}`, producing invalid
ClickHouse dot/backtick JSON syntax; replace that pattern with explicit
JSONExtract calls like JSONExtractString(props, <escaped key>) for string
filters, JSONExtractFloat64 or toFloat64(JSONExtractString(...)) for numeric
filters, and an appropriate JSONExtract for boolean (or compare to
'true'/'false' via JSONExtractString) when building goalCondition; update the
code that appends to goalCondition in the filters loop (referencing
propValueAccessor and goalCondition) to use JSONExtract* functions and
SqlString.escape(filter.value) as before; apply the same fix pattern used in
getErrorNames.ts and mirror the change in the other affected modules getGoals,
getFunnel, and getFunnelStepSessions.
🧹 Nitpick comments (5)
client/src/app/[site]/goals/components/GoalFormModal.tsx (3)
137-162: FormdefaultValuesmissingpropertyFiltersin config.When editing or cloning a goal, the form's
defaultValuesdon't includepropertyFilters. While you manage this via separate React state (propertyFilters), this creates a split source of truth. The form'sconfigobject won't reflect the actual filters until submission.This works because you're using local state for the UI and only syncing to form values on submit, but it's worth noting for maintainability.
346-404: Consider extracting duplicated property filters UI into a reusable component.The property filters UI for path goals (lines 346-404) and event goals (lines 433-483) are nearly identical, differing only in placeholder text. Extracting this into a shared component would reduce duplication and make future changes easier.
♻️ Suggested extraction
interface PropertyFiltersInputProps { filters: Array<{ key: string; value: string }>; onChange: (filters: Array<{ key: string; value: string }>) => void; keyPlaceholder: string; valuePlaceholder: string; addButtonText: string; } function PropertyFiltersInput({ filters, onChange, keyPlaceholder, valuePlaceholder, addButtonText }: PropertyFiltersInputProps) { return ( <div className="space-y-3"> {filters.map((filter, index) => ( <div key={index} className="flex gap-2 items-start"> <div className="flex-1 grid grid-cols-2 gap-2"> <Input placeholder={keyPlaceholder} value={filter.key} onChange={e => { const newFilters = [...filters]; newFilters[index].key = e.target.value; onChange(newFilters); }} /> <Input placeholder={valuePlaceholder} value={filter.value} onChange={e => { const newFilters = [...filters]; newFilters[index].value = e.target.value; onChange(newFilters); }} /> </div> {filters.length > 1 && ( <Button type="button" variant="ghost" size="icon" onClick={() => onChange(filters.filter((_, i) => i !== index))} > <X className="h-4 w-4" /> </Button> )} </div> ))} <Button type="button" variant="outline" size="sm" onClick={() => onChange([...filters, { key: "", value: "" }])} className="w-full" > <Plus className="mr-2 h-4 w-4" /> {addButtonText} </Button> </div> ); }Also applies to: 433-483
215-218: ResetpropertyFiltersstate after successful submission.After successful form submission,
usePropertiesis reset butpropertyFiltersis not reset here. It's only reset inonOpenChange(line 233). If the modal is immediately reopened for creating a new goal, stale filters could persist briefly beforeonOpenChangefires.♻️ Proposed fix
// Reset form and state after successful submission form.reset(); setUseProperties(false); + setPropertyFilters([{ key: "", value: "" }]); setIsOpen(false);server/src/api/analytics/goals/createGoal.ts (1)
43-46: Consider adding minimum length validation for property filter keys.The
keyfield inpropertyFiltersaccepts empty strings, which could lead to invalid filter configurations being stored. Consider adding.min(1)validation to ensure keys are non-empty.♻️ Proposed fix
propertyFilters: z.array(z.object({ - key: z.string(), + key: z.string().min(1, "Property key cannot be empty"), value: z.union([z.string(), z.number(), z.boolean()]), })).optional(),server/src/api/analytics/goals/updateGoal.ts (1)
46-49: Same validation improvement recommended as increateGoal.ts.For consistency and data integrity, consider adding
.min(1)validation to thekeyfield to prevent empty property keys.♻️ Proposed fix
propertyFilters: z.array(z.object({ - key: z.string(), + key: z.string().min(1, "Property key cannot be empty"), value: z.union([z.string(), z.number(), z.boolean()]), })).optional(),
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
client/src/api/analytics/endpoints/goals.tsclient/src/api/analytics/hooks/goals/useUpdateGoal.tsclient/src/app/[site]/goals/components/GoalFormModal.tsxserver/src/api/analytics/goals/createGoal.tsserver/src/api/analytics/goals/getGoalSessions.tsserver/src/api/analytics/goals/updateGoal.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- client/src/api/analytics/endpoints/goals.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript with strict typing throughout both client and server
Use camelCase for variables and functions
Use PascalCase for components and types
Group imports by external, then internal (alphabetical within groups)
Error handling: Use try/catch blocks with specific error types
Files:
server/src/api/analytics/goals/createGoal.tsclient/src/api/analytics/hooks/goals/useUpdateGoal.tsclient/src/app/[site]/goals/components/GoalFormModal.tsxserver/src/api/analytics/goals/getGoalSessions.tsserver/src/api/analytics/goals/updateGoal.ts
client/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Client: React functional components with minimal useEffect and inline functions
Files:
client/src/api/analytics/hooks/goals/useUpdateGoal.tsclient/src/app/[site]/goals/components/GoalFormModal.tsx
client/**/*.{css,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Dark mode is default theme
Files:
client/src/app/[site]/goals/components/GoalFormModal.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build Client Images (ubuntu-latest, linux/amd64)
- GitHub Check: Build Client Images (ubuntu-24.04-arm, linux/arm64)
🔇 Additional comments (4)
client/src/api/analytics/hooks/goals/useUpdateGoal.ts (2)
15-18: LGTM!The
propertyFilterstype definition correctly supports the new multi-filter feature with proper typing for the value union type.
33-39: LGTM!Including
siteIdin the mutation payload aligns with the server-sideupdateGoalSchemawhich now requiressiteIdin the request body for validation.client/src/app/[site]/goals/components/GoalFormModal.tsx (1)
71-83: State initialization may cause stale data when switching between goals.The initial state values for
usePropertiesandpropertyFiltersare computed from thegoalprop at component mount time. Whengoalchanges (e.g., editing a different goal), these initial values won't update until theuseEffectruns. However, theuseEffectdependency on[isOpen, goal]should handle this when the modal opens.Consider the edge case where
goalprop changes while the modal is already open (unlikely but possible). The current implementation should be fine for typical usage patterns.server/src/api/analytics/goals/getGoalSessions.ts (1)
58-70: LGTM on the backward compatibility approach.The fallback logic correctly handles both the new
propertyFiltersarray and legacy single-property fields, ensuring existing goals continue to work.
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 (1)
server/src/api/analytics/funnels/getFunnelStepSessions.ts (1)
130-146: Critical:url_parametersis missing from theSessionActionsCTE but referenced in step conditions.The page step conditions at line 95 reference
url_parameters[...], but theSessionActionsCTE (lines 130-146) doesn't includeurl_parametersin its SELECT list. This will cause query failures when page steps have property filters.Add
url_parametersto the CTE:🐛 Proposed fix
SessionActions AS ( SELECT session_id, timestamp, pathname, event_name, type, props, - hostname + hostname, + url_parameters FROM events WHERE site_id = {siteId:Int32} ${timeStatement} ${filterStatement} ),
🤖 Fix all issues with AI agents
In @server/src/api/analytics/funnels/getFunnel.ts:
- Around line 96-112: The code builds JSON accessors using dot notation with
SqlString.escapeId(filter.key) (see propValueAccessor in the filters loop) which
is less explicit for dynamic string keys; change propValueAccessor to use
bracket notation with the escaped string key (e.g.,
props[${SqlString.escape(filter.key)}]) and update the constructed conditions
that use propValueAccessor (the toString, toFloat64OrNull, toUInt8OrNull
comparisons) to reference the new bracket-style accessor so dynamic JSON keys
are handled clearly and consistently.
In @server/src/api/analytics/funnels/getFunnelStepSessions.ts:
- Around line 109-119: The filter loop currently builds prop access with
SqlString.escapeId (backticks), which breaks ClickHouse; change the
propValueAccessor in getFunnelStepSessions.ts to use bracket notation like
props[${SqlString.escape(filter.key)}] (not SqlString.escapeId) and keep the
existing type conversions (toString(...), toFloat64OrNull(...),
toUInt8OrNull(...)) applied to that bracket expression; apply the identical fix
in the other files mentioned (getFunnel.ts, getGoals.ts, getGoalSessions.ts)
wherever event property access uses SqlString.escapeId so all property accesses
use bracket notation with SqlString.escape.
🧹 Nitpick comments (1)
server/src/api/analytics/funnels/createFunnel.ts (1)
8-19: Add deprecation comment for legacy fields for consistency.In
getFunnel.ts, theeventPropertyKeyandeventPropertyValuefields are annotated with a deprecation comment. Consider adding the same comment here for consistency across the codebase:type FunnelStep = { value: string; name?: string; type: "page" | "event"; hostname?: string; + // Deprecated fields - kept for backwards compatibility eventPropertyKey?: string; eventPropertyValue?: string | number | boolean; + // New field for multiple property filters propertyFilters?: Array<{ key: string; value: string | number | boolean; }>; };
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
server/src/api/analytics/funnels/createFunnel.tsserver/src/api/analytics/funnels/getFunnel.tsserver/src/api/analytics/funnels/getFunnelStepSessions.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript with strict typing throughout both client and server
Use camelCase for variables and functions
Use PascalCase for components and types
Group imports by external, then internal (alphabetical within groups)
Error handling: Use try/catch blocks with specific error types
Files:
server/src/api/analytics/funnels/createFunnel.tsserver/src/api/analytics/funnels/getFunnel.tsserver/src/api/analytics/funnels/getFunnelStepSessions.ts
🧬 Code graph analysis (1)
server/src/api/analytics/funnels/getFunnelStepSessions.ts (1)
server/src/api/analytics/utils/utils.ts (1)
patternToRegex(100-115)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build Client Images (ubuntu-latest, linux/amd64)
- GitHub Check: Build Client Images (ubuntu-24.04-arm, linux/arm64)
🔇 Additional comments (6)
server/src/api/analytics/funnels/createFunnel.ts (1)
27-110: LGTM!The function correctly handles both create and update operations for funnels. The new
propertyFiltersfield will be persisted alongside other step data. Input validation occurs at query time ingetFunnel.tsandgetFunnelStepSessions.ts.server/src/api/analytics/funnels/getFunnelStepSessions.ts (2)
9-20: LGTM!The
FunnelSteptype is consistent with the other funnel files. Consider adding deprecation comments foreventPropertyKey/eventPropertyValueas done ingetFunnel.tsfor documentation consistency.
79-97: LGTM!The page step filtering correctly:
- Unifies
propertyFiltersand legacy single-property fields into a common array- Uses
SqlString.escapefor safe SQL construction- Converts values to strings appropriately since URL parameters are stored as strings in ClickHouse
server/src/api/analytics/funnels/getFunnel.ts (3)
8-21: LGTM!Good documentation with deprecation comments for the legacy fields. The type structure is clear and well-organized.
61-83: LGTM!The page step filtering correctly handles:
- Regex pattern escaping with manual quote handling (necessary to preserve regex syntax)
- Unified filter array construction for backward compatibility
- URL parameter matching with proper string conversion
124-142: LGTM!The
SessionActionsCTE correctly includes bothhostnameandurl_parametersin the SELECT list, making them available for the step conditions. The query structure is well-organized with clear CTE naming.
| for (const filter of filters) { | ||
| const propValueAccessor = `props.${SqlString.escapeId(filter.key)}`; | ||
|
|
||
| if (typeof step.eventPropertyValue === "string") { | ||
| condition += ` AND toString(${propValueAccessor}) = ${SqlString.escape(step.eventPropertyValue)}`; | ||
| } else if (typeof step.eventPropertyValue === "number") { | ||
| condition += ` AND toFloat64OrNull(${propValueAccessor}) = ${SqlString.escape(step.eventPropertyValue)}`; | ||
| } else if (typeof step.eventPropertyValue === "boolean") { | ||
| condition += ` AND toUInt8OrNull(${propValueAccessor}) = ${step.eventPropertyValue ? 1 : 0}`; | ||
| if (typeof filter.value === "string") { | ||
| condition += ` AND toString(${propValueAccessor}) = ${SqlString.escape(filter.value)}`; | ||
| } else if (typeof filter.value === "number") { | ||
| condition += ` AND toFloat64OrNull(${propValueAccessor}) = ${SqlString.escape(filter.value)}`; | ||
| } else if (typeof filter.value === "boolean") { | ||
| condition += ` AND toUInt8OrNull(${propValueAccessor}) = ${filter.value ? 1 : 0}`; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how JSON properties are accessed in the analytics code
rg -n "props\." server/src/api/analytics --type ts | head -40Repository: rybbit-io/rybbit
Length of output: 670
🏁 Script executed:
#!/bin/bash
# Look for SqlString usage patterns in the codebase
rg -n "SqlString\." server/src/api/analytics --type ts -A 1 | head -50Repository: rybbit-io/rybbit
Length of output: 4352
🏁 Script executed:
#!/bin/bash
# Find the file in question to see full context
cat -n server/src/api/analytics/funnels/getFunnelStepSessions.ts | head -130Repository: rybbit-io/rybbit
Length of output: 5887
🌐 Web query:
ClickHouse JSON property access syntax documentation
💡 Result:
Here are the ClickHouse docs and a concise summary of JSON property-access syntax and functions.
Key docs
- JSON functions reference (JSONExtract, JSONExtractString/Raw, JSONHas, JSONLength, etc.). [1]
- JSON path / JSON Query functions (JSON_EXISTS, JSON_QUERY, JSON_VALUE — use JSONPath strings like '$.a[0].b'). [2]
- Native JSON column type and notes on accessing nested subcolumns (including .^ syntax for sub-objects). [3]
Quick summary of access patterns
- Functional access (most common): JSONExtract(json, return_type), JSONExtractString/Int/Raw, JSONHas, JSONLength, etc. Keys/indices are passed as additional arguments: JSONExtract(json, 'key', 1, 'Int32') or JSONExtractString(json, 'a', 'b'). [1]
- indices_or_keys semantics: each index/key argument can be a string (object member) or integer (1‑based index into array/object); negative integers count from the end. (Minimum index = 1). [1]
- JSON path functions: JSON_EXISTS(json, path), JSON_QUERY(json, path), JSON_VALUE(json, path) accept a JSONPath-like string (e.g. '$.hello[0].world'). Use these when you prefer path expressions. [2]
- Case-insensitive variants: functions with CaseInsensitive suffix (e.g. JSONExtractStringCaseInsensitive) are available in newer versions. [1][3]
- JSON column specifics: ClickHouse has a native JSON column type with its own subcolumn access conventions (and .^ for reading nested sub-objects); prefer type hints for performance. [3]
If you want example queries for a specific access pattern (functional vs JSONPath) or your ClickHouse version, tell me which and I’ll provide them with exact syntax.
🌐 Web query:
npm sql-string escapeId backticks behavior
💡 Result:
Short answer — sqlstring (npm / mysqljs) escapeId behavior:
- escapeId wraps identifiers in backticks: e.g. SqlString.escapeId('col') ->
col. [1] - If the identifier contains a dot it treats it as qualified (schema/table/column) and escapes each part: SqlString.escapeId('table.col') ->
table.col(pass second arg true to prevent splitting: escapeId('a.b', true) ->a.b). [1] - Arrays become comma-separated escaped identifiers: escapeId(['c1','c2']) ->
c1,c2. [1] - Any backtick inside an identifier is escaped by doubling it (so an identifier containing ` becomes `` inside the wrapped backticks). This is the library’s established escaping behavior for identifiers (see sqlstring/mysqljs docs and implementation source). [1][2]
Sources: mysqljs / sqlstring documentation. [1] mysqljs/mysql README (Escaping query identifiers). [2] mysqljs/sqlstring package/readme.
Use bracket notation with SqlString.escape() for event property access, matching the page step pattern.
SqlString.escapeId() produces MySQL-style backticks (`key`), which ClickHouse does not support for property access. This causes SQL syntax errors for keys with special characters.
The page step code correctly uses bracket notation (line 95): url_parameters[${SqlString.escape(filter.key)}]. Event step property access should follow the same pattern:
- const propValueAccessor = `props.${SqlString.escapeId(filter.key)}`;
+ const propValueAccessor = `props[${SqlString.escape(filter.key)}]`;This same issue exists in getFunnel.ts (line 98), getGoals.ts (line 198), and getGoalSessions.ts (line 88). Update all four files consistently.
📝 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.
| for (const filter of filters) { | |
| const propValueAccessor = `props.${SqlString.escapeId(filter.key)}`; | |
| if (typeof step.eventPropertyValue === "string") { | |
| condition += ` AND toString(${propValueAccessor}) = ${SqlString.escape(step.eventPropertyValue)}`; | |
| } else if (typeof step.eventPropertyValue === "number") { | |
| condition += ` AND toFloat64OrNull(${propValueAccessor}) = ${SqlString.escape(step.eventPropertyValue)}`; | |
| } else if (typeof step.eventPropertyValue === "boolean") { | |
| condition += ` AND toUInt8OrNull(${propValueAccessor}) = ${step.eventPropertyValue ? 1 : 0}`; | |
| if (typeof filter.value === "string") { | |
| condition += ` AND toString(${propValueAccessor}) = ${SqlString.escape(filter.value)}`; | |
| } else if (typeof filter.value === "number") { | |
| condition += ` AND toFloat64OrNull(${propValueAccessor}) = ${SqlString.escape(filter.value)}`; | |
| } else if (typeof filter.value === "boolean") { | |
| condition += ` AND toUInt8OrNull(${propValueAccessor}) = ${filter.value ? 1 : 0}`; | |
| } | |
| } | |
| for (const filter of filters) { | |
| const propValueAccessor = `props[${SqlString.escape(filter.key)}]`; | |
| if (typeof filter.value === "string") { | |
| condition += ` AND toString(${propValueAccessor}) = ${SqlString.escape(filter.value)}`; | |
| } else if (typeof filter.value === "number") { | |
| condition += ` AND toFloat64OrNull(${propValueAccessor}) = ${SqlString.escape(filter.value)}`; | |
| } else if (typeof filter.value === "boolean") { | |
| condition += ` AND toUInt8OrNull(${propValueAccessor}) = ${filter.value ? 1 : 0}`; | |
| } | |
| } |
🤖 Prompt for AI Agents
In @server/src/api/analytics/funnels/getFunnelStepSessions.ts around lines 109 -
119, The filter loop currently builds prop access with SqlString.escapeId
(backticks), which breaks ClickHouse; change the propValueAccessor in
getFunnelStepSessions.ts to use bracket notation like
props[${SqlString.escape(filter.key)}] (not SqlString.escapeId) and keep the
existing type conversions (toString(...), toFloat64OrNull(...),
toUInt8OrNull(...)) applied to that bracket expression; apply the identical fix
in the other files mentioned (getFunnel.ts, getGoals.ts, getGoalSessions.ts)
wherever event property access uses SqlString.escapeId so all property accesses
use bracket notation with SqlString.escape.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
server/src/api/analytics/funnels/getFunnelStepSessions.ts (1)
85-117: Consider extracting duplicate filter logic into a shared utility.The filter derivation pattern (lines 87-91, 102-106) and the type-aware condition building (lines 109-116) are repeated identically in
getFunnel.ts,getGoalSessions.ts, andgetGoals.ts. Extracting these into shared utilities would reduce duplication and ensure consistent behavior.♻️ Example helper functions
// In a shared utils file, e.g., server/src/api/analytics/utils/propertyFilters.ts type PropertyFilter = { key: string; value: string | number | boolean }; export function resolvePropertyFilters( propertyFilters?: PropertyFilter[], legacyKey?: string, legacyValue?: string | number | boolean ): PropertyFilter[] { return propertyFilters || ( legacyKey && legacyValue !== undefined ? [{ key: legacyKey, value: legacyValue }] : [] ); } export function buildEventPropertyCondition(filters: PropertyFilter[]): string { let condition = ""; for (const filter of filters) { if (typeof filter.value === "string") { condition += ` AND JSONExtractString(toString(props), ${SqlString.escape(filter.key)}) = ${SqlString.escape(filter.value)}`; } else if (typeof filter.value === "number") { condition += ` AND toFloat64(JSONExtractString(toString(props), ${SqlString.escape(filter.key)})) = ${SqlString.escape(filter.value)}`; } else if (typeof filter.value === "boolean") { condition += ` AND JSONExtractString(toString(props), ${SqlString.escape(filter.key)}) = ${SqlString.escape(filter.value ? 'true' : 'false')}`; } } return condition; }server/src/api/analytics/funnels/getFunnel.ts (1)
64-67: Inconsistent regex escaping compared to getFunnelStepSessions.ts.This file uses manual quote escaping (
regex.replace(/'/g, "\\'")), whilegetFunnelStepSessions.tsusesSqlString.escape()for the same purpose. The manual approach may not handle all edge cases (e.g., backslashes).Consider using
SqlString.escape()consistently for regex escaping across both files.♻️ Suggested fix
if (step.type === "page") { // Use pattern matching for page paths to support wildcards const regex = patternToRegex(step.value); - // Manually escape single quotes in the regex and wrap in quotes - // Don't use SqlString.escape() as it doesn't preserve the regex correctly - const safeRegex = regex.replace(/'/g, "\\'"); - condition = `type = 'pageview' AND match(pathname, '${safeRegex}')`; + condition = `type = 'pageview' AND match(pathname, ${SqlString.escape(regex)})`;server/src/api/analytics/goals/getGoals.ts (1)
156-172: Variable namefiltersshadows the query parameterfilters.The local variable
filters(line 159) shadows thefiltersdestructured fromrequest.query(line 48). While this doesn't cause a bug due to scoping, it could lead to confusion during maintenance.Consider renaming to
propertyFiltersorgoalFiltersfor clarity.♻️ Suggested rename
- // Support both new propertyFilters array and legacy single property - const filters = goal.config.propertyFilters || ( + // Support both new propertyFilters array and legacy single property + const propertyFilters = goal.config.propertyFilters || ( goal.config.eventPropertyKey && goal.config.eventPropertyValue !== undefined ? [{ key: goal.config.eventPropertyKey, value: goal.config.eventPropertyValue }] : [] ); // Add property matching for page goals (URL parameters) - for (const filter of filters) { + for (const filter of propertyFilters) {Apply the same rename in the event goal section (lines 188-204).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
server/src/api/analytics/funnels/getFunnel.tsserver/src/api/analytics/funnels/getFunnelStepSessions.tsserver/src/api/analytics/goals/getGoalSessions.tsserver/src/api/analytics/goals/getGoals.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript with strict typing throughout both client and server
Use camelCase for variables and functions
Use PascalCase for components and types
Group imports by external, then internal (alphabetical within groups)
Error handling: Use try/catch blocks with specific error types
Files:
server/src/api/analytics/goals/getGoals.tsserver/src/api/analytics/funnels/getFunnel.tsserver/src/api/analytics/funnels/getFunnelStepSessions.tsserver/src/api/analytics/goals/getGoalSessions.ts
🧠 Learnings (1)
📚 Learning: 2025-12-31T18:19:49.611Z
Learnt from: CR
Repo: rybbit-io/rybbit PR: 0
File: client/src/api/CLAUDE.md:0-0
Timestamp: 2025-12-31T18:19:49.611Z
Learning: Applies to client/src/api/api/**/hooks/**/*.ts : Use descriptive array keys for React Query query keys (e.g., `['get-site', siteId]`) in hooks.
Applied to files:
server/src/api/analytics/funnels/getFunnelStepSessions.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build Client Images (ubuntu-24.04-arm, linux/arm64)
- GitHub Check: Build Client Images (ubuntu-latest, linux/amd64)
🔇 Additional comments (8)
server/src/api/analytics/funnels/getFunnelStepSessions.ts (2)
16-20: LGTM - Type extension for multi-property filtering.The
propertyFilterstype is well-defined and aligns with the PR objective to support filtering on pageview properties.
138-139: LGTM - SessionActions CTE correctly exposes url_parameters.Adding
url_parametersto the CTE projection is necessary to support the new URL parameter filtering for page steps.server/src/api/analytics/funnels/getFunnel.ts (3)
13-20: LGTM - Clear deprecation comment for legacy fields.Good practice to document that
eventPropertyKey/eventPropertyValueare deprecated while maintaining backward compatibility.
68-104: LGTM - Filter logic correctly handles both page and event steps.The implementation properly supports both the new
propertyFiltersarray and legacy single-property fields, with appropriate type-aware handling for event properties.
127-128: LGTM - SessionActions CTE updated for URL parameter filtering.server/src/api/analytics/goals/getGoalSessions.ts (2)
58-70: LGTM - Path goal filtering correctly accesses URL parameters.The filter logic properly derives filters from
propertyFilterswith legacy fallback, and correctly accessesurl_parametersfrom the events table in thegoalCondition.
79-95: LGTM - Event goal filtering with type-aware property matching.The implementation correctly handles string, number, and boolean property values using appropriate ClickHouse JSON extraction functions.
server/src/api/analytics/goals/getGoals.ts (1)
188-204: LGTM - Event goal filter logic is consistent with other files.The type-aware property matching correctly handles string, number, and boolean values.
1 participant
Added filtering for pageview url parameters on goals and funnels
Summary by CodeRabbit
New Features
Chores
Compatibility
✏️ Tip: You can customize this high-level summary in your review settings.