Add GoalCard expansion feature and session display#685
Conversation
Implemented an expandable GoalCard component that fetches and displays goal sessions when expanded. Added pagination controls for session navigation and integrated a loading state with a skeleton UI. Updated API routes to support fetching goal sessions. Refactored existing code for improved clarity and functionality.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedThe pull request is closed. WalkthroughAdds server endpoints and client hooks to fetch paginated sessions for goals and funnel steps, refactors funnel analytics to use session identifiers, and enhances Goal and Funnel UIs with expandable per-item session lists, tabs, pagination, and skeletons; also updates SessionsList to a props-driven pagination component. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GoalCard as GoalCard (Client)
participant GoalHook as useGetGoalSessions (Hook)
participant API as Frontend API
participant ServerG as Server (getGoalSessions)
participant CH as ClickHouse
User->>GoalCard: Click expand
GoalCard->>GoalHook: enable query (goalId, siteId, timeParams, page)
GoalHook->>API: GET /api/goals/:goalId/sessions/:site?time&page&limit
API->>ServerG: forward request
ServerG->>CH: execute session aggregation query
CH-->>ServerG: session rows
ServerG-->>API: { data: [...] }
API-->>GoalHook: response
GoalHook-->>GoalCard: render sessions + pagination
sequenceDiagram
participant User
participant FunnelStep as FunnelStepComponent (Client)
participant FunnelHook as useGetFunnelStepSessions (Hook)
participant API as Frontend API
participant ServerF as Server (getFunnelStepSessions)
participant CH as ClickHouse
User->>FunnelStep: Expand step / switch tab (Reached/Dropped)
FunnelStep->>FunnelHook: POST /api/funnel/:stepNumber/sessions/:site { steps } + params
FunnelHook->>API: request body + query params (time, mode, page, limit)
API->>ServerF: forward request
ServerF->>CH: execute CTE-based session tracing query
CH-->>ServerF: session rows
ServerF-->>API: { data: [...] }
API-->>FunnelHook: response
FunnelHook-->>FunnelStep: render session list and pagination
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
client/src/app/[site]/goals/components/GoalCard.tsx (1)
221-241: Consider accessibility for pagination controls.Ensure pagination buttons have appropriate
aria-labelattributes for screen readers, especially when only icons are displayed. Consider adding labels like "Previous page" and "Next page".Apply this diff to improve accessibility:
<Button variant="ghost" size="smIcon" + aria-label="Previous page" onClick={e => { e.stopPropagation(); setPage(p => p - 1); }} disabled={!hasPrevPage} > <ChevronLeft className="h-4 w-4" /> </Button> <span className="text-sm text-neutral-400">Page {page}</span> <Button variant="ghost" size="smIcon" + aria-label="Next page" onClick={e => { e.stopPropagation(); setPage(p => p + 1); }} disabled={!hasNextPage} > <ChevronRight className="h-4 w-4" /> </Button>server/src/api/analytics/goals/getGoalSessions.ts (2)
160-163: Error logging could be more informative.Consider logging additional context to aid debugging, such as the goalId, site, and page being requested.
Apply this diff to improve error logging:
} catch (error) { - console.error("Error fetching goal sessions:", error); + console.error("Error fetching goal sessions:", { goalId, site, page, limit, error }); return res.status(500).send({ error: "Failed to fetch goal sessions" }); }
87-146: Consider query performance for large datasets.The query joins
eventswithGoalSessionsonsession_id. For sites with high traffic, this could be resource-intensive. Consider:
- Adding appropriate indexes on
session_idin ClickHouse if not already present- Monitoring query performance in production
- If performance becomes an issue, consider materializing goal conversions during event ingestion
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
client/src/api/analytics/goals/useGetGoalSessions.ts(1 hunks)client/src/app/[site]/goals/components/GoalCard.tsx(6 hunks)client/src/app/[site]/goals/page.tsx(2 hunks)server/src/api/analytics/goals/getGoalSessions.ts(1 hunks)server/src/index.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
client/src/api/analytics/goals/useGetGoalSessions.ts (2)
client/src/components/DateSelector/types.ts (1)
Time(44-44)client/src/api/utils.ts (2)
getQueryParams(36-52)authedFetch(54-96)
server/src/index.ts (1)
server/src/api/analytics/goals/getGoalSessions.ts (1)
getGoalSessions(22-164)
server/src/api/analytics/goals/getGoalSessions.ts (4)
shared/src/params.ts (1)
FilterParams(10-10)server/src/db/postgres/schema.ts (1)
goals(247-272)server/src/api/analytics/utils.ts (2)
getTimeStatement(7-65)processResults(67-85)server/src/db/clickhouse/clickhouse.ts (1)
clickhouse(4-8)
client/src/app/[site]/goals/components/GoalCard.tsx (4)
client/src/api/analytics/goals/useGetGoals.ts (1)
Goal(6-20)client/src/lib/store.ts (1)
useStore(24-148)client/src/api/analytics/goals/useGetGoalSessions.ts (1)
useGetGoalSessions(10-38)client/src/components/Sessions/SessionCard.tsx (2)
SessionCardSkeleton(159-228)SessionCard(37-157)
⏰ 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 (15)
client/src/app/[site]/goals/page.tsx (2)
17-57: LGTM! Skeleton component extraction improves maintainability.The dedicated
GoalCardSkeletoncomponent is well-structured and uses appropriate Tailwind classes to mirror the actual goal card layout during loading states.
154-154: Verify skeleton display during site loading.The condition
isLoading || !sitewill show skeletons when the site is not yet loaded. Ensure that thesitevalue fromuseStore()is properly initialized to prevent unnecessary skeleton flashing on initial render or when switching sites.server/src/index.ts (3)
40-40: LGTM! Import statement is correct.The import of
getGoalSessionsfollows the established pattern for analytics goal endpoints.
242-242: LGTM! Route pattern added for public analytics access.The route pattern
/api/goals/:goalId/sessions/is correctly added toANALYTICS_ROUTES, enabling the same public access logic as other analytics endpoints (via direct access, public sites, or valid private keys).
335-335: LGTM! Route registration follows established patterns.The route
GET /api/goals/:goalId/sessions/:siteis correctly wired to thegetGoalSessionshandler, consistent with other analytics endpoints.client/src/api/analytics/goals/useGetGoalSessions.ts (1)
1-38: LGTM! Hook structure is well-designed.The
useGetGoalSessionshook follows React Query best practices:
- Query key properly includes all dependencies for cache invalidation
- Enabled condition prevents unnecessary requests
- Default values are sensible (page=1, limit=25, enabled=false)
- Uses existing utility functions (
authedFetch,getQueryParams)client/src/app/[site]/goals/components/GoalCard.tsx (5)
30-30: Good practice defining the limit as a constant.The
LIMITconstant improves maintainability by centralizing the pagination limit value.
34-47: LGTM! Efficient data fetching with expansion state.The hook is correctly configured to fetch sessions only when the card is expanded (
enabled: isExpanded), preventing unnecessary API calls.
63-68: LGTM! Expansion toggle resets pagination.Resetting to page 1 when expanding provides a consistent user experience.
73-200: LGTM! Clickable header with proper event handling.The header area correctly:
- Uses cursor-pointer and hover state for visual feedback
- Has
stopPropagationon action buttons to prevent unintended expansion- Includes a dedicated expand/collapse button with tooltip for discoverability
211-260: LGTM! Well-structured expanded section with proper states.The expanded sessions section properly handles:
- Loading state with skeleton UI
- Empty state with helpful message
- Pagination controls with disabled states
- Session list rendering
server/src/api/analytics/goals/getGoalSessions.ts (4)
22-43: LGTM! Proper validation and authorization.The handler correctly:
- Fetches the goal from PostgreSQL
- Returns 404 if goal not found
- Verifies the goal belongs to the specified site (403 if mismatch)
50-83: LGTM! Comprehensive goal condition building.The goal matching logic properly handles:
- Path goals with regex pattern matching via
patternToRegex- Event goals with exact event name matching
- Optional event property matching with type-specific conversions (string, number, boolean)
- Appropriate 400 errors for invalid configurations
87-146: LGTM! Well-structured ClickHouse query.The query design is solid:
- Uses CTEs for readability (GoalSessions → AggregatedSessions)
- Efficiently filters matching sessions first, then joins for aggregation
- Employs appropriate ClickHouse functions (argMax, argMin, countIf) for session-level aggregations
- Orders by session_end DESC for most recent sessions first
- Properly parameterized to prevent SQL injection
148-159: LGTM! Proper query execution and result processing.The query execution:
- Uses parameterized queries for safety
- Applies correct pagination calculation:
(page - 1) * limit- Defaults limit to 25, matching frontend expectations
- Processes results with the shared
processResultsutility
Updated the Funnel component to include a new SessionsTabContent for displaying session details with pagination controls. Improved the structure of the FunnelStepComponent for better readability and maintainability. Added API route for fetching funnel step sessions and adjusted SQL queries for session-based tracking. Removed unused skeleton code for a cleaner implementation.
…pe for regex safety Updated the SQL condition for funnel step sessions to utilize SqlString.escape for regex patterns, ensuring proper handling of special characters. This change enhances security and maintains the integrity of the regex in the query.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
client/src/api/analytics/funnels/useGetFunnelStepSessions.ts(1 hunks)client/src/app/[site]/funnels/components/Funnel.tsx(3 hunks)client/src/app/[site]/funnels/page.tsx(1 hunks)server/src/api/analytics/funnels/getFunnel.ts(4 hunks)server/src/api/analytics/funnels/getFunnelStepSessions.ts(1 hunks)server/src/index.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/src/index.ts
🧰 Additional context used
🧬 Code graph analysis (4)
client/src/app/[site]/funnels/components/Funnel.tsx (3)
client/src/components/Sessions/SessionCard.tsx (2)
SessionCardSkeleton(159-228)SessionCard(37-157)client/src/lib/store.ts (1)
useStore(24-148)client/src/api/analytics/funnels/useGetFunnelStepSessions.ts (1)
useGetFunnelStepSessions(11-51)
client/src/api/analytics/funnels/useGetFunnelStepSessions.ts (3)
client/src/api/analytics/funnels/useGetFunnel.ts (1)
FunnelStep(8-15)client/src/components/DateSelector/types.ts (1)
Time(44-44)client/src/api/utils.ts (2)
getQueryParams(36-52)authedFetch(54-96)
server/src/api/analytics/funnels/getFunnelStepSessions.ts (4)
client/src/api/analytics/funnels/useGetFunnel.ts (1)
FunnelStep(8-15)shared/src/params.ts (1)
FilterParams(10-10)server/src/api/analytics/utils.ts (2)
getTimeStatement(7-65)processResults(67-85)server/src/db/clickhouse/clickhouse.ts (1)
clickhouse(4-8)
server/src/api/analytics/funnels/getFunnel.ts (1)
server/src/api/analytics/utils.ts (1)
patternToRegex(297-312)
⏰ 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)
| return useQuery({ | ||
| queryKey: ["funnel-step-sessions", steps, stepNumber, siteId, timeParams, mode, page, limit], | ||
| queryFn: async () => { | ||
| return authedFetch<FunnelStepSessionsResponse>( | ||
| `/funnel/${stepNumber}/sessions/${siteId}`, | ||
| { | ||
| ...timeParams, | ||
| mode, | ||
| page, | ||
| limit, | ||
| }, | ||
| { | ||
| method: "POST", | ||
| data: { steps }, | ||
| } | ||
| ); | ||
| }, | ||
| enabled: !!siteId && !!steps && steps.length >= 2 && enabled, |
There was a problem hiding this comment.
Plumb funnel filters into the sessions query
The funnel summary already respects the user’s active filters, but this hook never forwards them (nor keys the cache with them), so the sessions panel will always show unfiltered data. Please thread the filters through the hook signature, add them to the query key, and include them in the request params before shipping. Import the existing filter type you use elsewhere to keep the typing consistent.
export function useGetFunnelStepSessions({
steps,
stepNumber,
siteId,
time,
mode,
page = 1,
limit = 25,
- enabled = false,
+ enabled = false,
+ filters = [],
}: {
steps: FunnelStep[];
stepNumber: number;
siteId: number;
time: Time;
mode: "reached" | "dropped";
page?: number;
limit?: number;
- enabled?: boolean;
+ enabled?: boolean;
+ filters?: Filter[];
}) {
const timeParams = getQueryParams(time);
return useQuery({
- queryKey: ["funnel-step-sessions", steps, stepNumber, siteId, timeParams, mode, page, limit],
+ queryKey: ["funnel-step-sessions", steps, stepNumber, siteId, timeParams, mode, page, limit, filters],
queryFn: async () => {
return authedFetch<FunnelStepSessionsResponse>(
`/funnel/${stepNumber}/sessions/${siteId}`,
{
...timeParams,
mode,
page,
- limit,
+ limit,
+ filters,
},
{
method: "POST",
data: { steps },
}
);
},
enabled: !!siteId && !!steps && steps.length >= 2 && enabled,
});
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In client/src/api/analytics/funnels/useGetFunnelStepSessions.ts around lines 32
to 49, the hook must accept and forward the active funnel filters: import the
existing filter type you use elsewhere (e.g., FunnelFilters or similar) and add
a filters parameter to the hook signature; include filters in the useQuery
queryKey (so cache varies by filters) and merge filters into the request params
passed to authedFetch alongside timeParams, mode, page, and limit; finally
ensure the enabled condition still checks siteId, steps and enabled but no extra
changes are required there.
| // Fetch sessions for "reached" mode | ||
| const { data: reachedData, isLoading: isLoadingReached } = useGetFunnelStepSessions({ | ||
| steps, | ||
| stepNumber: step.stepNumber, | ||
| siteId, | ||
| time, | ||
| mode: "reached", | ||
| page: reachedPage, | ||
| limit: LIMIT, | ||
| enabled: isExpanded && currentTab === "reached", | ||
| }); | ||
|
|
||
| // Fetch sessions for "dropped" mode (only if not first step) | ||
| // For step N, we want sessions that reached step N-1 but NOT step N | ||
| const { data: droppedData, isLoading: isLoadingDropped } = useGetFunnelStepSessions({ | ||
| steps, | ||
| stepNumber: step.stepNumber - 1, // Previous step | ||
| siteId, | ||
| time, | ||
| mode: "dropped", | ||
| page: droppedPage, | ||
| limit: LIMIT, | ||
| enabled: isExpanded && currentTab === "dropped" && !isFirstStep, | ||
| }); |
There was a problem hiding this comment.
Pass active filters into the sessions hook
Once the hook/server handle filters, this component still never supplies them, so the requests would stay unfiltered. Grab filters from the store alongside time and forward them to both useGetFunnelStepSessions calls.
- const { time } = useStore();
+ const { time, filters } = useStore();
...
limit: LIMIT,
- enabled: isExpanded && currentTab === "reached",
+ enabled: isExpanded && currentTab === "reached",
+ filters,
});
...
limit: LIMIT,
- enabled: isExpanded && currentTab === "dropped" && !isFirstStep,
+ enabled: isExpanded && currentTab === "dropped" && !isFirstStep,
+ filters,
});Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In client/src/app/[site]/funnels/components/Funnel.tsx around lines 122 to 145,
the sessions hooks aren't receiving the active filters so requests remain
unfiltered; retrieve `filters` from the same store where `time` is obtained
(e.g. alongside `time`) and pass that `filters` value into the options for both
useGetFunnelStepSessions calls (reached and dropped), keeping all other params
and enabled conditions unchanged so the hooks request filtered data.
… improved session management Added a new useGetSessions hook to fetch user sessions with pagination support. Refactored the SessionsList component to accept sessions as props, enhancing its reusability across different components. Updated various components to utilize the new SessionsList implementation, ensuring consistent session display and pagination controls. Improved loading states and error handling for better user experience.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
client/src/components/Sessions/SessionCard.tsx (1)
179-179: Increased skeleton count aligns with paginated session display.The change from 10 to 25 skeleton items is appropriate for the new pagination feature.
Consider making the skeleton count configurable to maintain consistency with the actual page size:
-export const SessionCardSkeleton = memo(() => { +export const SessionCardSkeleton = memo(({ count = 25 }: { count?: number } = {}) => { // Function to get a random width class for skeletons const getRandomWidth = () => { const widths = ["w-16", "w-20", "w-24", "w-28", "w-32", "w-36", "w-40", "w-44", "w-48"]; return widths[Math.floor(Math.random() * widths.length)]; }; // Get random width for time displays const getRandomTimeWidth = () => { const widths = ["w-20", "w-24", "w-28", "w-32"]; return widths[Math.floor(Math.random() * widths.length)]; }; // Get random width for duration displays const getRandomDurationWidth = () => { const widths = ["w-10", "w-12", "w-14"]; return widths[Math.floor(Math.random() * widths.length)]; }; // Create multiple skeletons for a realistic loading state - const skeletons = Array.from({ length: 25 }).map((_, index) => ( + const skeletons = Array.from({ length: count }).map((_, index) => (Alternatively, define a shared constant if 25 is used as a page size elsewhere in the codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
client/src/api/analytics/useGetUserSessions.ts(1 hunks)client/src/app/[site]/funnels/components/Funnel.tsx(3 hunks)client/src/app/[site]/globe/components/GlobeSessions.tsx(2 hunks)client/src/app/[site]/goals/components/GoalCard.tsx(6 hunks)client/src/app/[site]/sessions/page.tsx(1 hunks)client/src/app/[site]/user/[userId]/page.tsx(4 hunks)client/src/components/Sessions/SessionCard.tsx(1 hunks)client/src/components/Sessions/SessionsList.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
client/src/api/analytics/useGetUserSessions.ts (5)
client/src/lib/store.ts (2)
useStore(24-148)getFilteredFilters(291-294)client/src/api/utils.ts (2)
getQueryParams(36-52)authedFetch(54-96)client/src/lib/filterGroups.ts (1)
SESSION_PAGE_FILTERS(29-29)client/src/api/types.ts (1)
APIResponse(1-4)client/src/lib/dateTimeUtils.ts (1)
timeZone(14-14)
client/src/app/[site]/globe/components/GlobeSessions.tsx (2)
client/src/api/analytics/useGetUserSessions.ts (1)
useGetSessionsInfinite(84-128)client/src/components/Sessions/SessionCard.tsx (1)
SessionCardSkeleton(159-228)
client/src/app/[site]/goals/components/GoalCard.tsx (5)
client/src/api/analytics/goals/useGetGoals.ts (1)
Goal(6-20)client/src/api/analytics/goals/useDeleteGoal.ts (1)
useDeleteGoal(5-26)client/src/lib/store.ts (1)
useStore(24-148)client/src/api/analytics/goals/useGetGoalSessions.ts (1)
useGetGoalSessions(10-38)client/src/components/Sessions/SessionsList.tsx (1)
SessionsList(18-59)
client/src/app/[site]/sessions/page.tsx (2)
client/src/api/analytics/useGetUserSessions.ts (1)
useGetSessions(46-82)client/src/components/Sessions/SessionsList.tsx (1)
SessionsList(18-59)
client/src/app/[site]/funnels/components/Funnel.tsx (5)
client/src/api/analytics/funnels/useGetFunnel.ts (1)
FunnelStep(8-15)client/src/lib/store.ts (1)
useStore(24-148)client/src/api/analytics/funnels/useGetFunnelStepSessions.ts (1)
useGetFunnelStepSessions(11-51)client/src/components/EventIcons.tsx (2)
PageviewIcon(4-6)EventIcon(8-10)client/src/components/Sessions/SessionsList.tsx (1)
SessionsList(18-59)
client/src/app/[site]/user/[userId]/page.tsx (2)
client/src/api/analytics/useGetUserSessions.ts (1)
useGetSessions(46-82)client/src/components/Sessions/SessionsList.tsx (1)
SessionsList(18-59)
client/src/components/Sessions/SessionsList.tsx (3)
client/src/api/analytics/useGetUserSessions.ts (1)
GetSessionsResponse(8-44)client/src/components/NothingFound.tsx (1)
NothingFound(1-20)client/src/components/Sessions/SessionCard.tsx (1)
SessionCardSkeleton(159-228)
⏰ 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 (12)
client/src/app/[site]/funnels/components/Funnel.tsx (2)
45-79: Forward active filters to the per-step session queriesThe step sessions ignore any filters the funnel applies because we never forward
filtersfrom the store into eitheruseGetFunnelStepSessionscall. That breaks parity with the chart and was already flagged earlier. Please pullfiltersalongsidetimeand pass them through so the fetched sessions stay in sync with the rest of the funnel UI.duplicate_commentraise_major_issue- const { time } = useStore(); + const { time, filters } = useStore(); … const { data: reachedData, isLoading: isLoadingReached } = useGetFunnelStepSessions({ steps, stepNumber: step.stepNumber, siteId, time, mode: "reached", page: reachedPage, limit: LIMIT, - enabled: isExpanded && currentTab === "reached", + filters, + enabled: isExpanded && currentTab === "reached", }); … const { data: droppedData, isLoading: isLoadingDropped } = useGetFunnelStepSessions({ steps, stepNumber: step.stepNumber - 1, siteId, time, mode: "dropped", page: droppedPage, limit: LIMIT, - enabled: isExpanded && currentTab === "dropped" && !isFirstStep, + filters, + enabled: isExpanded && currentTab === "dropped" && !isFirstStep, });
84-87: Tighten next-page detectionUsing
hasNextPage = reachedSessions.length === LIMIT(and the dropped analogue) misfires whenever the total count is an exact multiple ofLIMIT, leaving users with a “next” button that pulls an empty page. Please switch to a more reliable check (e.g., requestLIMIT + 1records and slice toLIMIT, or return total-count metadata from the API).raise_minor_issueclient/src/api/analytics/useGetUserSessions.ts (1)
54-79: Include thelimitparameter in the requestThe new
limitargument is never sent to the backend, so every caller still receives the API’s default page size (25 today). That breaks pagination—for example,SessionsPageexpects 100 items, sees only 25, and disables “next” even when more data exists. Please forwardlimitwith the request.raise_major_issueconst requestParams: Record<string, any> = { time_zone: timeZone, filters: filteredFilters, page, + limit, };client/src/app/[site]/goals/components/GoalCard.tsx (1)
58-223: Handle the last page correctly
hasNextPage = totalSessions === LIMITstill yields a phantom next page when the total count is an exact multiple of 25—the very edge case we discussed earlier. Please adopt one of the suggested fixes (e.g., requestLIMIT + 1items and slice, or use total-count metadata) so the pagination controls stay accurate.duplicate_commentraise_minor_issueclient/src/app/[site]/user/[userId]/page.tsx (3)
36-37: LGTM! Reasonable pagination limit.The LIMIT of 25 sessions per page provides good balance between performance and UX.
44-44: LGTM! Proper pagination state initialization.Starting from page 1 is correct for initial load.
235-243: LGTM! SessionsList integration is complete and correct.All required props are properly passed with appropriate values.
client/src/components/Sessions/SessionsList.tsx (5)
7-16: LGTM! Well-structured props interface.The interface clearly defines all required pagination and data props with appropriate types.
18-27: LGTM! Clean refactor to controlled component pattern.The transition from default export to named export with explicit props follows React best practices and makes the component more testable and reusable.
28-34: LGTM! Proper empty state handling.The condition
sessions.length === 0 && !isLoadingcorrectly prevents showing the empty state during initial load.
39-47: LGTM! Pagination controls are well implemented.The disabled states for navigation buttons are correctly tied to
hasPrevPageandhasNextPageprops, and the current page indicator provides good user feedback.
50-56: LGTM! Rendering logic correctly handles loading and data states.The skeleton display during loading and subsequent session card rendering provide good UX. The composite key
${session.session_id}-${index}ensures stable rendering even if session IDs repeat across pages.
…and improve pagination Updated the useGetFunnel and useGetFunnelStepSessions hooks to utilize FUNNEL_PAGE_FILTERS for enhanced filtering capabilities. Adjusted the Funnel component and GoalCard to support pagination with an additional session limit. Improved session handling in various components to ensure consistent data display and navigation. Enhanced SQL queries in the backend to accommodate new filter statements for funnel step sessions.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
client/src/app/[site]/goals/components/GoalCard.tsx (2)
73-76: Consider adding accessibility attributes to the clickable header.The header region is interactive but lacks keyboard navigation support and ARIA attributes. Users relying on keyboard navigation cannot expand/collapse the card.
Apply this diff to improve accessibility:
<div className="px-4 py-3 flex items-center mb-1 cursor-pointer hover:bg-neutral-800/50 transition-colors" onClick={toggleExpansion} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + toggleExpansion(); + } + }} + role="button" + tabIndex={0} + aria-expanded={isExpanded} >
211-225: Consider adding error state handling for the sessions query.The expanded section handles loading and empty states well, but if the
useGetGoalSessionsquery fails, users see "No sessions found" without any indication that an error occurred. This conflates "no data exists" with "failed to fetch data."Destructure the error from the query and conditionally render an error message:
-const { data: sessionsData, isLoading: isLoadingSessions } = useGetGoalSessions({ +const { data: sessionsData, isLoading: isLoadingSessions, error: sessionsError } = useGetGoalSessions({ goalId: goal.goalId, siteId, time, page, limit: LIMIT + 1, enabled: isExpanded, });Then in the expanded section:
{isExpanded && ( <div className="border-t border-neutral-800 bg-neutral-900/50 p-4"> <h4 className="text-sm font-medium text-neutral-200 mb-3">Converted Sessions</h4> + {sessionsError && ( + <div className="text-sm text-red-400 mb-3"> + Failed to load sessions. Please try again. + </div> + )} <SessionsList sessions={sessions} isLoading={isLoadingSessions}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
client/src/api/analytics/funnels/useGetFunnel.ts(2 hunks)client/src/api/analytics/funnels/useGetFunnelStepSessions.ts(1 hunks)client/src/app/[site]/funnels/components/Funnel.tsx(3 hunks)client/src/app/[site]/goals/components/GoalCard.tsx(6 hunks)client/src/app/[site]/sessions/page.tsx(1 hunks)client/src/app/[site]/user/[userId]/page.tsx(4 hunks)client/src/lib/filterGroups.ts(1 hunks)server/src/api/analytics/funnels/getFunnelStepSessions.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- client/src/app/[site]/sessions/page.tsx
🧰 Additional context used
🧬 Code graph analysis (6)
client/src/app/[site]/goals/components/GoalCard.tsx (5)
client/src/api/analytics/goals/useGetGoals.ts (1)
Goal(6-20)client/src/api/analytics/goals/useDeleteGoal.ts (1)
useDeleteGoal(5-26)client/src/lib/store.ts (1)
useStore(24-148)client/src/api/analytics/goals/useGetGoalSessions.ts (1)
useGetGoalSessions(10-38)client/src/components/Sessions/SessionsList.tsx (1)
SessionsList(18-59)
client/src/app/[site]/funnels/components/Funnel.tsx (5)
client/src/api/analytics/funnels/useGetFunnel.ts (1)
FunnelStep(8-15)client/src/lib/store.ts (1)
useStore(24-148)client/src/api/analytics/funnels/useGetFunnelStepSessions.ts (1)
useGetFunnelStepSessions(14-57)client/src/components/EventIcons.tsx (2)
PageviewIcon(4-6)EventIcon(8-10)client/src/components/Sessions/SessionsList.tsx (1)
SessionsList(18-59)
client/src/api/analytics/funnels/useGetFunnel.ts (3)
client/src/lib/store.ts (2)
useStore(24-148)getFilteredFilters(291-294)client/src/api/utils.ts (1)
getQueryParams(36-52)client/src/lib/filterGroups.ts (1)
FUNNEL_PAGE_FILTERS(42-42)
client/src/api/analytics/funnels/useGetFunnelStepSessions.ts (5)
client/src/api/analytics/funnels/useGetFunnel.ts (1)
FunnelStep(8-15)client/src/components/DateSelector/types.ts (1)
Time(44-44)client/src/api/utils.ts (2)
getQueryParams(36-52)authedFetch(54-96)client/src/lib/store.ts (1)
getFilteredFilters(291-294)client/src/lib/filterGroups.ts (1)
FUNNEL_PAGE_FILTERS(42-42)
server/src/api/analytics/funnels/getFunnelStepSessions.ts (4)
client/src/api/analytics/funnels/useGetFunnel.ts (1)
FunnelStep(8-15)shared/src/params.ts (1)
FilterParams(10-10)server/src/api/analytics/utils.ts (3)
getTimeStatement(7-65)getFilterStatement(141-288)processResults(67-85)server/src/db/clickhouse/clickhouse.ts (1)
clickhouse(4-8)
client/src/app/[site]/user/[userId]/page.tsx (2)
client/src/api/analytics/useGetUserSessions.ts (2)
useGetUserSessionCount(232-245)useGetSessions(46-82)client/src/components/Sessions/SessionsList.tsx (1)
SessionsList(18-59)
⏰ 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 (23)
client/src/app/[site]/user/[userId]/page.tsx (3)
3-3: LGTM! Import updates align with pagination requirements.The import changes correctly support the new pagination implementation: SessionsList as a named export, useState for page state, and useGetSessions for fetching paginated data.
Also applies to: 18-18, 20-20
36-36: LGTM! Pagination constants and state are correctly initialized.The LIMIT of 25 sessions per page is reasonable, and the page state correctly starts at 1.
Also applies to: 44-44
236-244: LGTM! SessionsList integration is correct.All required props are properly passed to SessionsList, including the pagination controls. The implementation matches the component's expected interface as shown in
client/src/components/Sessions/SessionsList.tsx.client/src/app/[site]/goals/components/GoalCard.tsx (5)
3-3: LGTM: Imports and pagination constant.All new imports are properly utilized, and the
LIMITconstant provides a clear, centralized pagination size.Also applies to: 7-7, 9-9, 22-22, 30-30
34-47: Well-structured state and data fetching.The
enabled: isExpandedflag ensures sessions are only fetched when needed, and usingtimefrom the store correctly integrates with the global time filter context. Thelimit: LIMIT + 1approach is a clean solution for detecting additional pages.
58-61: Pagination edge case correctly resolved.The
LIMIT + 1fetch strategy with slice-based display accurately detects whether more pages exist, eliminating the false "next page" button issue flagged in the previous review.
141-146: LGTM: Event propagation correctly prevented.The
stopPropagationcalls on Edit, Clone, and Delete buttons prevent unintended expansion when interacting with these controls. The inline comments clearly document the intent.Also applies to: 162-167, 178-182
191-209: LGTM: Visual indicators and progress bar.The chevron button provides clear visual feedback for the expansion state, and the refactored progress bar structure correctly displays the conversion rate with proper relative/absolute positioning.
client/src/api/analytics/funnels/useGetFunnel.ts (1)
4-5: LGTM—Centralized filter management.The refactor to use
FUNNEL_PAGE_FILTERSandgetFilteredFiltersaligns with the broader pattern established across funnel queries in this PR and ensures consistent filter application.Also applies to: 40-44
client/src/lib/filterGroups.ts (1)
42-43: LGTM—Dedicated funnel filter constant.Introducing
FUNNEL_PAGE_FILTERSprovides a clear, dedicated constant for funnel filtering and allows future customization if funnel filters need to diverge from the base set.client/src/api/analytics/funnels/useGetFunnelStepSessions.ts (1)
37-56: LGTM—Query implementation is correct.The query key correctly includes all relevant parameters (including
filteredFilters), the request properly forwards filters to the server, and the enabled condition appropriately gates the query.client/src/app/[site]/funnels/components/Funnel.tsx (6)
4-11: LGTM—Setup for expandable step functionality.The imports, constant definition, and props interface are well-structured for the new per-step session exploration feature.
Also applies to: 29-38
40-55: LGTM—State and metric calculations.State management for expansion, tabs, and pagination is appropriate. The bar width and dropoff calculations correctly derive metrics from the chart data.
Note: The past review comment suggesting to pass
filtersto the hooks is no longer relevant, asuseGetFunnelStepSessionsnow derives filters from the store internally.
56-79: LGTM—Session queries are correctly structured.The dual queries for "reached" and "dropped" modes are properly configured:
- "Reached" queries sessions completing the current step
- "Dropped" correctly queries
stepNumber - 1to find sessions that reached the previous step but not the current one- Conditional enabling appropriately gates the queries
81-98: LGTM—Pagination logic is correct.The pagination implementation correctly uses the
LIMIT + 1fetch strategy to detecthasNext, slices results for display, and appropriately resets state when toggling expansion.
100-201: LGTM—Render implementation is comprehensive.The component rendering is well-structured with:
- Interactive header with expansion controls
- Dual-bar visualization for relative and absolute conversion
- Tabbed session views with appropriate conditional rendering
- Proper pagination and empty state handling
203-242: LGTM—Funnel refactoring maintains clarity.The refactor to extract per-step rendering into
FunnelStepComponentmaintains clean separation of concerns while preserving existing funnel metrics and visualization logic.server/src/api/analytics/funnels/getFunnelStepSessions.ts (6)
1-32: LGTM—Type definitions and imports.The type definitions appropriately match the client-side types, and the request interface is properly structured for Fastify.
34-57: LGTM—Comprehensive request validation.The validation logic appropriately handles edge cases, including the early return for "dropped" mode on the final step where no drop-off is possible.
59-101: LGTM—Filter and condition building.The filter retrieval and UTM column transformation are correct—the transformation is necessary because the
AggregatedSessionsCTE extracts UTM parameters as separate columns (lines 196-200). Step condition building appropriately handles page/event types and property filtering.
103-167: LGTM—Funnel CTE construction is correct.The CTE chain correctly implements funnel progression tracking:
SessionActionsfilters events by site, time, and filtersStep1throughStepNenforce temporal ordering viatimestamp > step_timeTargetSessionscorrectly distinguishes "reached" vs "dropped" sessionsThe logic aligns with the client-side convention where "dropped" mode queries
stepNumber - 1(Funnel.tsx line 72).
169-227: Verify the double filter application semantics.Filters are applied in two places:
- Line 119: Within
SessionActionsto filter events used for funnel step matching- Line 225: On aggregated session attributes in the final
SELECTWhile the past review confirmed filters should be applied to
SessionActions, the second application may cause subtle inconsistencies. For example, if a session has events from multiple countries and the filter iscountry=US:
SessionActionssees only US events → session qualifies for funnelAggregatedSessionsaggregates all session events →argMax(country)might return a non-US country- Final
WHEREfilters out the session → session vanishes from results despite qualifyingIs this the intended behavior, or should
AggregatedSessionsalso filter events byfilterStatement?If the current behavior is correct, consider adding a comment explaining why filters are applied twice and what the semantic difference is.
229-244: LGTM—Query execution and error handling.The parameterized query execution prevents SQL injection, pagination calculations are correct, and error handling appropriately logs and returns server errors.
…gination support Updated the useGetUserSessions hook to include a limit parameter for pagination. Cleaned up imports in useGetFunnelStepSessions by removing unused variables and ensuring consistent filter handling. These changes improve session management and maintainability across analytics components.
Implemented an expandable GoalCard component that fetches and displays goal sessions when expanded. Added pagination controls for session navigation and integrated a loading state with a skeleton UI. Updated API routes to support fetching goal sessions. Refactored existing code for improved clarity and functionality.
Summary by CodeRabbit
New Features
Refactor