Implement outbound link as a goal type#823
Implement outbound link as a goal type#823stijnie2210 wants to merge 2 commits intorybbit-io:masterfrom
Conversation
|
@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 a new "outbound" goal type across client, server, and DB: API types and UI updated to capture outboundUrlPattern; server-side validation and query logic extended to handle outbound pattern matching; DB schema/config updated to store outboundUrlPattern. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant DB
Client->>Server: POST /goals { goalType: "outbound", config:{ outboundUrlPattern } }
Server->>Server: validate body (outboundUrlPattern required for outbound)
Server->>DB: INSERT goal record with config.outboundUrlPattern
DB-->>Server: inserted goal
Server-->>Client: 201 Created
Note right of Server: Later, querying conversions
Client->>Server: GET /goals?siteId=...
Server->>DB: Aggregate events: COUNT IF(type='outbound' AND match(url, regex_from(outboundUrlPattern)))
DB-->>Server: aggregation results
Server-->>Client: return goals + conversion counts
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
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 (1)
client/src/api/analytics/endpoints/goals.ts (1)
53-66: MissingoutboundUrlPatterninCreateGoalParams.config.The
Goalinterface includesoutboundUrlPatternin its config (line 21), butCreateGoalParams.configdoesn't include it. This will cause TypeScript errors when attempting to create outbound goals, as the field won't be allowed in the request payload.🐛 Proposed fix
export interface CreateGoalParams { name?: string; goalType: "path" | "event" | "outbound"; config: { pathPattern?: string; eventName?: string; eventPropertyKey?: string; eventPropertyValue?: string | number | boolean; propertyFilters?: Array<{ key: string; value: string | number | boolean; }>; + outboundUrlPattern?: string; }; }
🤖 Fix all issues with AI agents
In `@client/src/app/`[site]/goals/components/GoalFormModal.tsx:
- Around line 234-254: In GoalFormModal fix the mismatched braces and duplicated
clearing logic in the block that normalizes values before submit: remove the
stray closing braces and the duplicated "Clear legacy fields" section, then
consolidate the per-goalType clearing logic so values.config only clears fields
not relevant to the current values.goalType (references: GoalFormModal,
values.goalType, values.config, propertyFilters, useProperties). Specifically
ensure the "event" branch sets pathPattern/outboundUrlPattern to undefined and
conditionally sets config.propertyFilters from propertyFilters when
useProperties is true, the "outbound" branch clears event-related fields
(eventName/eventPropertyKey/eventPropertyValue), and finally perform a single,
well-placed cleanup that clears any legacy eventPropertyKey/eventPropertyValue
only when they are not applicable.
🧹 Nitpick comments (1)
client/src/app/[site]/goals/components/GoalCard.tsx (1)
89-107: Consider explicit type check instead of implicit else.The
elsebranch implicitly assumes any goal that isn't "path" or "event" must be "outbound". This could lead to incorrect rendering if additional goal types are introduced in the future.♻️ Suggested improvement
- ) : goal.goalType === "event" ? ( + ) : goal.goalType === "event" ? ( <Tooltip> <TooltipTrigger asChild> <EventIcon /> </TooltipTrigger> <TooltipContent> <p>Event Goal</p> </TooltipContent> </Tooltip> - ) : ( + ) : goal.goalType === "outbound" ? ( <Tooltip> <TooltipTrigger asChild> <OutboundIcon /> </TooltipTrigger> <TooltipContent> <p>Outbound Goal</p> </TooltipContent> </Tooltip> + ) : null} )}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@client/src/app/`[site]/goals/components/GoalFormModal.tsx:
- Around line 183-186: The second useEffect that sets useProperties (triggered
when isOpen/goal changes) only checks legacy fields
(goal?.config.eventPropertyKey && goal?.config.eventPropertyValue) and
overwrites the correct state set earlier; update this effect (or consolidate
both useEffects into one) so it mirrors the logic used earlier by checking both
the new propertyFilters array (goal?.config.propertyFilters?.length) and the
legacy fields before calling setUseProperties, ensuring useProperties is true if
either the propertyFilters array has entries or both legacy fields exist (and
thus avoiding the race/overwrite).
| useEffect(() => { | ||
| if (isOpen) { | ||
| setUseProperties(!!goal?.config.eventPropertyKey && !!goal?.config.eventPropertyValue); | ||
|
|
There was a problem hiding this comment.
Inconsistent useProperties initialization overwrites correct state.
Line 185 only checks legacy fields (eventPropertyKey && eventPropertyValue), but the earlier useEffect at lines 130-133 correctly checks both propertyFilters?.length and legacy fields. Since both effects run on isOpen/goal changes, this effect runs second and overwrites the correct value, causing the property filters UI to be hidden when editing goals that use the new propertyFilters array.
🐛 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);
if ((isEditMode || isCloneMode) && goal) {Alternatively, consider consolidating these two useEffects into one to avoid the race condition and reduce duplication.
📝 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.
| useEffect(() => { | |
| if (isOpen) { | |
| setUseProperties(!!goal?.config.eventPropertyKey && !!goal?.config.eventPropertyValue); | |
| useEffect(() => { | |
| if (isOpen) { | |
| const hasFilters = !!( | |
| goal?.config.propertyFilters?.length || | |
| (goal?.config.eventPropertyKey && goal?.config.eventPropertyValue !== undefined) | |
| ); | |
| setUseProperties(hasFilters); | |
| if ((isEditMode || isCloneMode) && goal) { |
🤖 Prompt for AI Agents
In `@client/src/app/`[site]/goals/components/GoalFormModal.tsx around lines 183 -
186, The second useEffect that sets useProperties (triggered when isOpen/goal
changes) only checks legacy fields (goal?.config.eventPropertyKey &&
goal?.config.eventPropertyValue) and overwrites the correct state set earlier;
update this effect (or consolidate both useEffects into one) so it mirrors the
logic used earlier by checking both the new propertyFilters array
(goal?.config.propertyFilters?.length) and the legacy fields before calling
setUseProperties, ensuring useProperties is true if either the propertyFilters
array has entries or both legacy fields exist (and thus avoiding the
race/overwrite).
1 participant
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.