[PULSE-36] Funnels UI - builder and report #8
Reference in New Issue
Block a user
No description provided.
Delete Branch "staging"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Work Item
PULSE-36
Summary
Changes
lib/api/funnels.ts– list, get, create, update, delete funnels; get funnel stats (optionalfrom/to).app/sites/[id]/funnels/page.tsx– list funnels, empty state, Create Funnel (btn-primary), delete with confirm.app/sites/[id]/funnels/new/page.tsx– form (name, description, steps: name/path/type exact|contains|regex), add/remove steps, Cancel (btn-secondary), submit (btn-primary).app/sites/[id]/funnels/[funnelId]/page.tsx– funnel name/description, date range, theme-aware bar chart, stats table (visitors, drop-off %, conversion %), delete; hooks (useTheme, useMemo) before early return (fix React #310).app/sites/[id]/page.tsx– Funnels button on site dashboard; back links and actions usebtn-primary/btn-secondaryandrounded-xl.useTheme+ CHART_COLORS_LIGHT/DARK; step pillsrounded-lg; empty state aligned with GoalStats.Test Plan
[ ] From site dashboard, open Funnels; list and empty state render.
[ ] Create funnel with multiple steps (exact, contains, regex); save and see it in list.
[ ] Open funnel report; date range changes update chart and table; drop-off/conversion values are sensible.
[ ] Delete funnel from list and from report; confirm flow works.
[ ] UI uses Ciphera buttons/radii and chart respects light/dark theme.
Greptile Overview
Greptile Summary
This PR implements a comprehensive funnels feature for the Pulse analytics dashboard, adding the ability to track user journeys through multi-step conversion paths.
Key additions:
lib/api/funnels.ts) with full CRUD operations and RFC3339 date normalization for UTC-aware date range queriesCode quality highlights:
useCallbackusage, correct dependency arrays, hooks before early returns)datePresetstate to avoid reference comparison issuesAll previously flagged issues from review threads have been addressed. The implementation follows existing Pulse patterns and integrates cleanly with the Ciphera branding system.
Confidence Score: 5/5
Important Files Changed
Sequence Diagram
7 files reviewed, 2 comments
Edit Code Review Agent Settings | Greptile
@@ -0,0 +1,331 @@'use client'ReferenceError on mount
useEffectcallsloadData()beforeloadDatais defined (const loadData = async () => { ... }), which throwsReferenceError: Cannot access 'loadData' before initializationon first render. Move theloadDatadefinition above theuseEffect(or switch to a hoistedfunction loadData() {}) so the page doesn’t crash.Prompt To Fix With AI
@@ -0,0 +1,149 @@'use client'ReferenceError on mount
Same pattern as the report page:
useEffectcallsloadFunnels()beforeconst loadFunnels = async () => { ... }is initialized, so this page will throwReferenceError: Cannot access 'loadFunnels' before initializationon mount. DefineloadFunnelsabove the effect (or use a hoisted function declaration).Prompt To Fix With AI
@@ -0,0 +1,149 @@'use client'Change: The loadFunnels definition was moved above the useEffect that calls it.
Why: Same pattern as the report page: the effect was calling loadFunnels() before the const loadFunnels = async () => { ... } was initialized. Defining loadFunnels before the useEffect fixes the ReferenceError on mount.
@@ -0,0 +1,331 @@'use client'Change: The loadData definition was moved above the useEffect that calls it.
Why: const loadData = async () => { ... } is not hoisted, so when the effect ran on the first render it was still in the temporal dead zone and threw “Cannot access 'loadData' before initialization”. Defining loadData before the useEffect ensures it exists when the effect runs.
4 files reviewed, 7 comments
Edit Code Review Agent Settings | Greptile
@@ -0,0 +1,331 @@'use client'Missing
loadDatain dependency array causes React exhaustive-deps warning and stale closure issues. Either addloadDatato the dependency array or useuseCallbackto memoizeloadData.Prompt To Fix With AI
Date comparison using string equality (
dateRange.start === getDateRange(7).start) creates new Date objects on every render, causing unnecessary re-renders and always evaluating to false since objects are different references.Prompt To Fix With AI
Unused import
useAuth- not referenced in the component.Prompt To Fix With AI
Opacity calculation can go negative for funnels with more than 6 steps, causing render issues. Wrap in
Math.max(0.1, ...)to prevent negative opacity.Prompt To Fix With AI
@@ -0,0 +1,235 @@'use client'Unused import
useAuth- not referenced in the component.Prompt To Fix With AI
@@ -0,0 +1,149 @@'use client'Missing
loadFunnelsin dependency array causes React exhaustive-deps warning and stale closure issues. Either addloadFunnelsto the dependency array or useuseCallbackto memoizeloadFunnels.Prompt To Fix With AI
Unused variable
userdestructured fromuseAuth()- not referenced in the component.Prompt To Fix With AI
@@ -0,0 +1,331 @@'use client'Change: Replaced fillOpacity={1 - (index * 0.15)} with fillOpacity={Math.max(0.1, 1 - index * 0.15)}.
Why: For index ≥ 7, 1 - index * 0.15 becomes negative; capping with Math.max(0.1, ...) keeps opacity in a valid range and avoids render issues.
@@ -0,0 +1,331 @@'use client'Change: Removed import { useAuth } from '@/lib/auth/context' from app/sites/[id]/funnels/[funnelId]/page.tsx.
Why: The report page doesn’t use useAuth.
@@ -0,0 +1,149 @@'use client'Change: Removed useAuth() and its import from app/sites/[id]/funnels/page.tsx (the list page).
Why: Only user was destructured and it wasn’t used, so the whole hook and import were removed.
@@ -0,0 +1,235 @@'use client'Change: Removed import { useAuth } from '@/lib/auth/context' from app/sites/[id]/funnels/new/page.tsx.
Why: The component doesn’t use useAuth.
@@ -0,0 +1,331 @@'use client'Change: Introduced datePreset state: '7' | '30' | 'custom', default '30'. The Select uses value={datePreset}. In onChange: for '7'/'30' we call setDateRange(getDateRange(7)) / setDateRange(getDateRange(30)) and setDatePreset('7') / setDatePreset('30'); for 'custom' we only open the picker. In the DatePicker onApply we call setDatePreset('custom') when applying a custom range.
Why: We no longer call getDateRange(7) / getDateRange(30) on every render to derive the Select value; the value comes from datePreset, so no extra Date allocations or reference issues.
@@ -0,0 +1,149 @@'use client'Change: Wrapped loadFunnels in useCallback with deps [siteId], and set the effect dependency array to [loadFunnels].
Why: Same as above: correct deps and no stale closure.
@@ -0,0 +1,331 @@'use client'Change: Wrapped the loader in useCallback with deps [siteId, funnelId, dateRange], and made the effect depend only on [loadData].
Why: loadData is now stable per (siteId, funnelId, dateRange), so the effect runs when any of those change and satisfies exhaustive-deps without stale closures.
7 files reviewed, 2 comments
Edit Code Review Agent Settings | Greptile
@@ -0,0 +1,331 @@'use client'import { useCallback, useEffect, useMemo, useState } from 'react'Unused auth import
useAuthis imported here but never referenced, which will fail lint/typecheck in projects enforcingno-unused-vars. Remove the import to avoid CI failures.Prompt To Fix With AI
@@ -0,0 +1,235 @@'use client'import { useState } from 'react'Unused auth import
useAuthis imported but not used in this page, which will tripno-unused-vars/typecheck in many setups. Remove the import.Prompt To Fix With AI
@@ -0,0 +1,331 @@'use client'import { useCallback, useEffect, useMemo, useState } from 'react'Already fixed.
@@ -0,0 +1,235 @@'use client'import { useState } from 'react'Already fixed.
3 files reviewed, 3 comments
Edit Code Review Agent Settings | Greptile
@@ -0,0 +34,4 @@}const handleRemoveStep = (index: number) => {if (steps.length <= 1) returnMinimum step validation (≥1) only happens in UI. Backend may require at least 2 steps for a funnel to be meaningful. Verify backend requirements.
Prompt To Fix With AI
@@ -0,0 +58,4 @@return}if (steps.some(s => !s.value.trim())) {Step validation only checks if
valueis empty but doesn't validate regex syntax whentype === 'regex'. Invalid regex will cause backend errors or runtime issues when matching. Consider adding client-side regex validation:Add a helper:
Prompt To Fix With AI
@@ -0,0 +71,4 @@return {from: DATE_ONLY_REGEX.test(from) ? `${from}T00:00:00.000Z` : from,to: DATE_ONLY_REGEX.test(to) ? `${to}T23:59:59.999Z` : to,}Date normalization hardcodes
T23:59:59.999Zfor end dates, but this may not account for user's timezone. Iftois "2024-02-04", this sets end to "2024-02-04T23:59:59.999Z" (UTC), which might exclude data from users in timezones ahead of UTC who accessed the site on Feb 4th local time. Consider if timezone handling is needed or document the UTC assumption.Prompt To Fix With AI
@@ -0,0 +71,4 @@return {from: DATE_ONLY_REGEX.test(from) ? `${from}T00:00:00.000Z` : from,to: DATE_ONLY_REGEX.test(to) ? `${to}T23:59:59.999Z` : to,}Change: Updated the JSDoc for toRFC3339Range to: Normalize date-only (YYYY-MM-DD) to RFC3339 for backend funnel stats API. Uses UTC for boundaries (API/server timestamps are UTC).
Why: It documents that the T23:59:59.999Z end-of-day is intentional UTC and that the API/server use UTC; no code change for timezone handling.
@@ -0,0 +34,4 @@}const handleRemoveStep = (index: number) => {if (steps.length <= 1) returnChange: Added a comment above the steps state: // * Backend requires at least one step (API binding min=1, DB rejects empty steps).
No change to the minimum step count: the backend uses min=1 (API binding:"required,min=1" and DB len(steps) < 1), and the UI already enforces that (e.g. “remove step” disabled when only one step).
Why: The comment records that the backend requirement is “at least 1 step” and that the UI matches it; no change to require 2 steps.
@@ -0,0 +58,4 @@return}if (steps.some(s => !s.value.trim())) {Change:
Added isValidRegex(pattern: string): boolean that does try { new RegExp(pattern); return true } catch { return false }.
After the “path for all steps” check, added a check: steps.some(s => s.type === 'regex' && !isValidRegex(s.value)). If true, show toast.error('Invalid regex in one or more steps. Check the pattern for steps with type "regex".') and return without submitting.
Why: Invalid regex for type === 'regex' is caught on the client so the user gets a clear error instead of a backend/runtime failure.
4 files reviewed, 1 comment
Edit Code Review Agent Settings | Greptile
@@ -0,0 +46,4 @@}const handleSubmit = async (e: React.FormEvent) => {e.preventDefault()Regex validation missing despite previous thread mentioning it was added. When
step.type === 'regex', invalid regex patterns will cause backend errors. The previous thread indicatedisValidRegexhelper and validation were added, but they're not present in the code.Prompt To Fix With AI
@@ -0,0 +46,4 @@}const handleSubmit = async (e: React.FormEvent) => {e.preventDefault()Already in place: The isValidRegex helper (lines 9–16) and regex validation for steps with type === 'regex' were already there.
Change made: The validation was updated to match the suggested style:
Use steps.find(s => s.type === 'regex' && !isValidRegex(s.value)) instead of steps.some(...).
If an invalid regex step is found, show toast.error(\Invalid regex pattern in step: ${invalidRegexStep.name}) so the user sees which step failed.
Why: Behavior is the same (invalid regex still blocks submit), but the error message now names the step, as in the review suggestion.
7 files reviewed, 2 comments
Edit Code Review Agent Settings | Greptile
@@ -0,0 +130,4 @@<div className="w-full max-w-6xl mx-auto px-4 sm:px-6 py-8"><p className="text-neutral-600 dark:text-neutral-400">Funnel not found</p></div>)Misleading error state
When
loadData()fails (network/5xx/permission),funnel/statsremainnulland the render falls through toif (!funnel || !stats)showing "Funnel not found". That message is only correct for a 404; for transient errors it incorrectly looks like a permanent missing resource and provides no retry path. Consider tracking an expliciterror/notFoundstate and only rendering "not found" on an actual 404 response.Prompt To Fix With AI
@@ -0,0 +65,4 @@const invalidRegexStep = steps.find(s => s.type === 'regex' && !isValidRegex(s.value))if (invalidRegexStep) {toast.error(`Invalid regex pattern in step: ${invalidRegexStep.name}`)returnMissing step name validation
handleSubmitvalidatesstep.value(and regex syntax) but never validatesstep.name. This allows creating funnels with empty/whitespace step names, which then render as blank labels in the funnels list/report (step.nameis displayed in multiple places). Add a check likesteps.some(s => !s.name.trim())and block submit with a toast so funnels stay usable.Prompt To Fix With AI
@@ -0,0 +65,4 @@const invalidRegexStep = steps.find(s => s.type === 'regex' && !isValidRegex(s.value))if (invalidRegexStep) {toast.error(`Invalid regex pattern in step: ${invalidRegexStep.name}`)returnChange: In handleSubmit, after the funnel name check and before the path check, added: if (steps.some(s => !s.name.trim())) { toast.error('Please enter a name for all steps'); return }.
Why: Steps with empty or whitespace-only names are rejected so funnel list/report labels stay usable.
@@ -0,0 +130,4 @@<div className="w-full max-w-6xl mx-auto px-4 sm:px-6 py-8"><p className="text-neutral-600 dark:text-neutral-400">Funnel not found</p></div>)Change:
Added loadError state: 'not_found' | 'error' | null, default null.
In loadData: at the start, setLoadError(null); in the catch, if error instanceof ApiError && error.status === 404 then setLoadError('not_found'), else setLoadError('error') and toast.error('Failed to load funnel data') (toast only for non-404).
Render logic:
loadError === 'not_found' or (!funnel && !stats && !loadError) → show "Funnel not found" (no retry).
loadError === 'error' → show "Failed to load funnel data" and a "Try again" button that calls loadData().
Otherwise, if !funnel || !stats → fallback "Funnel not found".
Why: 404 is shown as “Funnel not found”; other failures (network/5xx/permission) are shown as “Failed to load funnel data” with a retry, so transient errors are no longer treated as a permanent missing resource.
3 files reviewed, 2 comments
Edit Code Review Agent Settings | Greptile
@@ -0,0 +95,4 @@return <LoadingOverlay logoSrc="/pulse_icon_no_margins.png" title="Pulse" />}if (loadError === 'not_found' || (!funnel && !stats && !loadError)) {Shows "Funnel not found" for all errors (404, 403, 500, network failures). For transient errors, this suggests the funnel doesn't exist when it might just be a temporary issue. Consider tracking error type separately to show appropriate messages (e.g., "Unable to load funnel" for 500/network, "Access denied" for 403).
Prompt To Fix With AI
@@ -0,0 +56,4 @@if (steps.some(s => !s.name.trim())) {toast.error('Please enter a name for all steps')return}Validates
step.valuebut notstep.name. Empty/whitespace step names will render as blank labels throughout the UI (list page line 121, report page line 246).Prompt To Fix With AI
@@ -0,0 +95,4 @@return <LoadingOverlay logoSrc="/pulse_icon_no_margins.png" title="Pulse" />}if (loadError === 'not_found' || (!funnel && !stats && !loadError)) {Change:
loadError is now 'not_found' | 'forbidden' | 'error' | null.
In loadData catch: status === 404 → setLoadError('not_found'); status === 403 → setLoadError('forbidden'); otherwise → setLoadError('error'). Toast only for non-404, non-403.
Render:
loadError === 'not_found' (or fallback) → "Funnel not found" (unchanged).
loadError === 'forbidden' → "Access denied" and a "Back to Funnels" link.
loadError === 'error' → "Unable to load funnel" and "Try again" button that calls loadData().
Why: 404 stays “Funnel not found”, 403 shows “Access denied” with a way back, and 500/network show “Unable to load funnel” with retry so transient errors are no longer treated as “not found”.
@@ -0,0 +56,4 @@if (steps.some(s => !s.name.trim())) {toast.error('Please enter a name for all steps')return}Status: Already implemented.
handleSubmit already has if (steps.some(s => !s.name.trim())) { toast.error('Please enter a name for all steps'); return } (lines 56–59), before the path check.