heatmaps #819
Open
heatmaps #819
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughThis PR introduces a heatmap visualization feature for analytics. It adds backend services and API endpoints to fetch click heatmap data and page lists, includes client-side React components for rendering heatmaps and controls, integrates click event capture from session replay, creates a database table for storing click events, and registers the feature in the analytics dashboard. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (Browser)
participant UI as Heatmap Page UI
participant Hook as React Hook<br/>(useGetClickHeatmap)
participant API as API Handler<br/>(fetchClickHeatmap)
participant Service as Backend Service<br/>(clickHeatmapService)
participant DB as ClickHouse DB
User->>UI: Navigate to Heatmaps, Select Page
UI->>Hook: Trigger query (pathname, viewport)
Hook->>API: Call fetchClickHeatmap
API->>Service: getClickHeatmap(siteId, pathname, options)
Service->>DB: Query session_replay_clicks<br/>(filtered by viewport, time, grid)
DB-->>Service: Click points aggregated by grid cell
Service->>DB: Query aggregate stats<br/>(totalClicks, uniqueSessions)
DB-->>Service: Aggregate results
Service-->>API: ClickHeatmapResult<br/>(points, stats)
API-->>Hook: Parsed response
Hook-->>UI: Update state (data/loading/error)
UI->>UI: Render HeatmapCanvas<br/>with points
UI-->>User: Display heatmap visualization
sequenceDiagram
participant Session as Session Replay<br/>(recordEvents)
participant Ingest as Replay Ingest Service<br/>(sessionReplayIngestService)
participant Extract as extractClickEvents<br/>(private)
participant DB as ClickHouse DB
Session->>Ingest: recordEvents(events, siteId, sessionId, viewport)
Ingest->>Extract: extractClickEvents(events, siteId, sessionId, viewport)
Extract->>Extract: Filter for MouseInteraction<br/>(type=3, source=2, type 2 or 4)
Extract->>Extract: Validate & format<br/>coordinates (x, y)
Extract-->>Ingest: Array of click objects
Ingest->>DB: INSERT INTO session_replay_clicks<br/>(site_id, session_id, timestamp, x, y, viewport_*, click_type)
DB-->>Ingest: Insert successful
Ingest-->>Session: recordEvents complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Fix all issues with AI agents
In `@client/src/api/analytics/hooks/heatmap/useGetClickHeatmap.ts`:
- Around line 19-33: The queryKey in useGetClickHeatmap is using the raw store
timezone variable while buildApiParams resolves it (via getTimezone()), causing
cache mismatches; update the hook so the queryKey uses the resolved timezone
value (call getTimezone() or otherwise obtain the same resolved timezone used by
buildApiParams) instead of the literal timezone from useStore(), or
alternatively change buildApiParams to accept an explicit timezone and pass
store.timezone into it so both the queryKey and the request params use the same
timezone resolution; locate symbols useGetClickHeatmap, useStore,
buildApiParams, getTimezone(), and the queryKey to make the change.
In `@client/src/app/`[site]/heatmaps/components/HeatmapControls.tsx:
- Around line 25-33: The segmented control buttons in HeatmapControls currently
lack an explicit button type and ARIA state; update the button element rendered
in HeatmapControls (the button with key={option.value} and onClick={() =>
onViewportChange(option.value)}) to include type="button" to prevent accidental
form submissions and add aria-pressed={viewportBreakpoint === option.value} (or
aria-pressed={String(viewportBreakpoint === option.value)}) so screen readers
can convey the toggle state; keep the existing className/cn logic and
onViewportChange handler unchanged.
In `@client/src/app/`[site]/heatmaps/components/HeatmapPageList.tsx:
- Around line 41-48: The button in HeatmapPageList.tsx should explicitly set
type="button" to avoid accidental form submissions and include an ARIA selection
state so screen readers know which page is selected; update the button rendered
in the map (the element using onSelectPage, selectedPathname and page.pathname)
to add type="button" and an appropriate ARIA attribute (e.g., aria-pressed or
aria-selected) set to the boolean expression selectedPathname === page.pathname.
In `@client/src/app/`[site]/heatmaps/components/HeatmapViewer.tsx:
- Around line 3-5: The iframeError/iframeLoaded state must be reset whenever the
viewed page changes to avoid persistent error/loading UI; in the HeatmapViewer
component add a useEffect that watches pageUrl and sets iframeError to false and
iframeLoaded to false when pageUrl changes, and ensure the iframe onLoad handler
(or the existing load success path) clears iframeError (sets it false) when the
iframe successfully loads; update references to iframeError and iframeLoaded so
the loading overlay and error UI reflect these state resets.
In `@client/src/app/`[site]/heatmaps/page.tsx:
- Line 24: Remove the debug console.info call that prints pagesData in page.tsx:
locate the console.info(pagesData) statement and delete it (or replace it with a
conditional debug-only logger gated by NODE_ENV === 'development' if you need
runtime debug). Ensure no other direct console logging of pagesData remains in
the component or related functions (search for pagesData usage) before merging.
In `@client/src/components/heatmap/HeatmapCanvas.tsx`:
- Around line 46-61: Guard against zero/invalid gridResolution and avoid
spreading a large array when computing maxValue: clamp gridResolution to a
minimum of 1 before converting coordinates (used in the x/y calculations where
gridResolution appears) to prevent Infinity, and replace
Math.max(...points.map((p) => p.value), 1) with a reducer that computes the
maxValue safely (e.g., points.reduce((m,p)=>Math.max(m,p.value), 1)) so you
don't spread the array; keep the existing alphaCanvas/alphaCtx handling but
ensure downstream intensity uses the computed maxValue.
In `@server/src/api/heatmap/getClickHeatmap.ts`:
- Around line 16-29: Validate inputs in getClickHeatmap before calling
clickHeatmapService: ensure siteId (derived from Number(req.params.siteId)) is a
valid positive integer (check isNaN and >0) and return res.status(400) with a
clear message if invalid; validate viewportBreakpoint from req.query against the
allowed enum values (explicitly list acceptable strings) and return 400 if it is
not one of them; coerce gridResolution to a Number, ensure it is a finite
integer within acceptable bounds (e.g., >0 and <= a sensible max like 1000) and
return 400 for out-of-range or non-integer values; only after these checks call
clickHeatmapService.getClickHeatmap with the validated/normalized values.
In `@server/src/api/heatmap/getHeatmapPages.ts`:
- Around line 14-22: Validate req.params.siteId and req.query.limit before
calling clickHeatmapService.getHeatmapPages: parse siteId from req.params.siteId
and ensure Number.isInteger(siteId) && siteId > 0, parse limit from
req.query.limit (fall back to 100) and ensure Number.isInteger(limit) && limit >
0; if either check fails, return a 400 response from getHeatmapPages with a
clear error message (use res.code(400).send(...)) instead of calling
clickHeatmapService.getHeatmapPages.
In `@server/src/services/heatmap/clickHeatmapService.ts`:
- Around line 134-169: In getHeatmapPages, the generated timeStatement
incorrectly references "sre.timestamp" while the query aliases
session_replay_clicks as src; update the replacement so
getTimeStatement(options).replace(/timestamp/g, "src.timestamp") (or otherwise
ensure timeStatement uses the src alias) so the WHERE clause matches the src
table alias used in the query's WHERE clause and joins.
- Around line 52-128: In getClickHeatmap replace the current srm.page_url LIKE
{pathnamePattern:String} filtering in both query and statsQuery with
path(srm.page_url) = {pathname:String} (so use ClickHouse's path() on
srm.page_url for exact-path matching), adjust the parameter you pass in
query_params from pathnamePattern to pathname (set to cleanPathname, with "/"
for root), and remove/stop using the LIKE-based pathnamePattern logic; update
references in clickhouse.query calls and query_params accordingly in the
getClickHeatmap function.
🧹 Nitpick comments (1)
server/src/services/replay/sessionReplayIngestService.ts (1)
120-136: Skip click inserts when viewport dimensions are missing.When viewportWidth/Height are 0, the extractor still generates rows that downstream queries immediately filter out, adding unnecessary write/storage overhead. Consider short‑circuiting when dimensions are unknown.
💡 Suggested guard
- const clicksToInsert = this.extractClickEvents( - events, - siteId, - sessionId, - metadata?.viewportWidth || 0, - metadata?.viewportHeight || 0 - ); + const viewportWidth = metadata?.viewportWidth; + const viewportHeight = metadata?.viewportHeight; + const clicksToInsert = + viewportWidth && viewportHeight + ? this.extractClickEvents(events, siteId, sessionId, viewportWidth, viewportHeight) + : [];
Comment on lines
+19
to
+33
| const { time, site, filters, timezone } = useStore(); | ||
| const params = buildApiParams(time, { filters }); | ||
|
|
||
| return useQuery({ | ||
| queryKey: ["click-heatmap", site, pathname, viewportBreakpoint, gridResolution, time, filters, timezone], | ||
| queryFn: () => | ||
| fetchClickHeatmap(site, { | ||
| ...params, | ||
| pathname, | ||
| viewportBreakpoint, | ||
| gridResolution, | ||
| }), | ||
| staleTime: 1000 * 60 * 5, // 5 minutes | ||
| refetchOnWindowFocus: false, | ||
| enabled: !!site && !!pathname && enabled, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find buildApiParams implementation
rg -n --type=ts "buildApiParams" -B2 -A10 | head -50Repository: rybbit-io/rybbit
Length of output: 4886
🏁 Script executed:
#!/bin/bash
# Find getTimezone implementation
rg -n --type=ts "getTimezone" -B2 -A8Repository: rybbit-io/rybbit
Length of output: 50373
🏁 Script executed:
#!/bin/bash
# Look for timezone handling in the analytics/hooks directory
fd . client/src/api/analytics -type f -name "*.ts" -o -name "*.tsx" | head -20Repository: rybbit-io/rybbit
Length of output: 230
Pass the resolved timezone to queryKey for cache consistency.
The queryKey should use the same resolved timezone value that buildApiParams uses internally. Currently, when store.timezone is "system", the queryKey includes the literal "system" string while the request params contain the resolved timezone from getTimezone(). If the resolved timezone changes at runtime, React Query will serve stale cached data with an outdated timezone.
Either include the resolved timezone in the queryKey (getTimezone() instead of timezone) or update buildApiParams to accept timezone as a parameter and pass store.timezone explicitly, ensuring queryKey and request params stay in sync.
🤖 Prompt for AI Agents
In `@client/src/api/analytics/hooks/heatmap/useGetClickHeatmap.ts` around lines 19
- 33, The queryKey in useGetClickHeatmap is using the raw store timezone
variable while buildApiParams resolves it (via getTimezone()), causing cache
mismatches; update the hook so the queryKey uses the resolved timezone value
(call getTimezone() or otherwise obtain the same resolved timezone used by
buildApiParams) instead of the literal timezone from useStore(), or
alternatively change buildApiParams to accept an explicit timezone and pass
store.timezone into it so both the queryKey and the request params use the same
timezone resolution; locate symbols useGetClickHeatmap, useStore,
buildApiParams, getTimezone(), and the queryKey to make the change.
Comment on lines
+25
to
+33
| <button | ||
| key={option.value} | ||
| onClick={() => onViewportChange(option.value)} | ||
| className={cn( | ||
| "flex items-center gap-1.5 px-3 py-1.5 text-sm rounded-md transition-colors", | ||
| viewportBreakpoint === option.value | ||
| ? "bg-white dark:bg-neutral-700 text-neutral-900 dark:text-neutral-100 shadow-sm" | ||
| : "text-neutral-600 dark:text-neutral-400 hover:text-neutral-900 dark:hover:text-neutral-200" | ||
| )} |
Contributor
There was a problem hiding this comment.
Add button type and ARIA state for the segmented control.
Improves a11y and avoids accidental form submission.
🛠️ Suggested tweak
- <button
+ <button
+ type="button"
+ aria-pressed={viewportBreakpoint === option.value}
key={option.value}
onClick={() => onViewportChange(option.value)}
className={cn(📝 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.
Suggested change
| <button | |
| key={option.value} | |
| onClick={() => onViewportChange(option.value)} | |
| className={cn( | |
| "flex items-center gap-1.5 px-3 py-1.5 text-sm rounded-md transition-colors", | |
| viewportBreakpoint === option.value | |
| ? "bg-white dark:bg-neutral-700 text-neutral-900 dark:text-neutral-100 shadow-sm" | |
| : "text-neutral-600 dark:text-neutral-400 hover:text-neutral-900 dark:hover:text-neutral-200" | |
| )} | |
| <button | |
| type="button" | |
| aria-pressed={viewportBreakpoint === option.value} | |
| key={option.value} | |
| onClick={() => onViewportChange(option.value)} | |
| className={cn( | |
| "flex items-center gap-1.5 px-3 py-1.5 text-sm rounded-md transition-colors", | |
| viewportBreakpoint === option.value | |
| ? "bg-white dark:bg-neutral-700 text-neutral-900 dark:text-neutral-100 shadow-sm" | |
| : "text-neutral-600 dark:text-neutral-400 hover:text-neutral-900 dark:hover:text-neutral-200" | |
| )} |
🤖 Prompt for AI Agents
In `@client/src/app/`[site]/heatmaps/components/HeatmapControls.tsx around lines
25 - 33, The segmented control buttons in HeatmapControls currently lack an
explicit button type and ARIA state; update the button element rendered in
HeatmapControls (the button with key={option.value} and onClick={() =>
onViewportChange(option.value)}) to include type="button" to prevent accidental
form submissions and add aria-pressed={viewportBreakpoint === option.value} (or
aria-pressed={String(viewportBreakpoint === option.value)}) so screen readers
can convey the toggle state; keep the existing className/cn logic and
onViewportChange handler unchanged.
Comment on lines
+41
to
+48
| <button | ||
| key={page.pathname} | ||
| onClick={() => onSelectPage(page.pathname)} | ||
| className={cn( | ||
| "flex items-center justify-between px-3 py-2 rounded-lg text-left transition-colors", | ||
| "hover:bg-neutral-100 dark:hover:bg-neutral-800", | ||
| selectedPathname === page.pathname && "bg-neutral-100 dark:bg-neutral-800 ring-1 ring-neutral-200 dark:ring-neutral-700" | ||
| )} |
Contributor
There was a problem hiding this comment.
Add button type and selection ARIA state for safety/accessibility.
Prevents accidental form submit and improves screen-reader state.
🛠️ Suggested tweak
- <button
+ <button
+ type="button"
+ aria-pressed={selectedPathname === page.pathname}
key={page.pathname}
onClick={() => onSelectPage(page.pathname)}
className={cn(📝 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.
Suggested change
| <button | |
| key={page.pathname} | |
| onClick={() => onSelectPage(page.pathname)} | |
| className={cn( | |
| "flex items-center justify-between px-3 py-2 rounded-lg text-left transition-colors", | |
| "hover:bg-neutral-100 dark:hover:bg-neutral-800", | |
| selectedPathname === page.pathname && "bg-neutral-100 dark:bg-neutral-800 ring-1 ring-neutral-200 dark:ring-neutral-700" | |
| )} | |
| <button | |
| type="button" | |
| aria-pressed={selectedPathname === page.pathname} | |
| key={page.pathname} | |
| onClick={() => onSelectPage(page.pathname)} | |
| className={cn( | |
| "flex items-center justify-between px-3 py-2 rounded-lg text-left transition-colors", | |
| "hover:bg-neutral-100 dark:hover:bg-neutral-800", | |
| selectedPathname === page.pathname && "bg-neutral-100 dark:bg-neutral-800 ring-1 ring-neutral-200 dark:ring-neutral-700" | |
| )} |
🤖 Prompt for AI Agents
In `@client/src/app/`[site]/heatmaps/components/HeatmapPageList.tsx around lines
41 - 48, The button in HeatmapPageList.tsx should explicitly set type="button"
to avoid accidental form submissions and include an ARIA selection state so
screen readers know which page is selected; update the button rendered in the
map (the element using onSelectPage, selectedPathname and page.pathname) to add
type="button" and an appropriate ARIA attribute (e.g., aria-pressed or
aria-selected) set to the boolean expression selectedPathname === page.pathname.
Comment on lines
+3
to
+5
| import { Loader2, MousePointerClick } from "lucide-react"; | ||
| import { useMemo, useState } from "react"; | ||
| import { useGetClickHeatmap } from "../../../../api/analytics/hooks/heatmap/useGetClickHeatmap"; |
Contributor
There was a problem hiding this comment.
Reset iframe state on page changes to avoid sticky error/loading UI.
If a page load fails once, iframeError stays true forever; and switching pages keeps iframeLoaded true, so the loading overlay never appears. Reset on pageUrl change and clear error on successful load.
🐛 Proposed fix
-import { useMemo, useState } from "react";
+import { useEffect, useMemo, useState } from "react";
@@
const [iframeLoaded, setIframeLoaded] = useState(false);
const [iframeError, setIframeError] = useState(false);
+ useEffect(() => {
+ setIframeLoaded(false);
+ setIframeError(false);
+ }, [pageUrl]);
@@
- onLoad={() => setIframeLoaded(true)}
- onError={() => setIframeError(true)}
+ onLoad={() => {
+ setIframeLoaded(true);
+ setIframeError(false);
+ }}
+ onError={() => {
+ setIframeLoaded(false);
+ setIframeError(true);
+ }}Also applies to: 24-90
🤖 Prompt for AI Agents
In `@client/src/app/`[site]/heatmaps/components/HeatmapViewer.tsx around lines 3 -
5, The iframeError/iframeLoaded state must be reset whenever the viewed page
changes to avoid persistent error/loading UI; in the HeatmapViewer component add
a useEffect that watches pageUrl and sets iframeError to false and iframeLoaded
to false when pageUrl changes, and ensure the iframe onLoad handler (or the
existing load success path) clears iframeError (sets it false) when the iframe
successfully loads; update references to iframeError and iframeLoaded so the
loading overlay and error UI reflect these state resets.
| const { data: siteData } = useGetSite(site); | ||
| const { data: pagesData, isLoading: isPagesLoading } = useGetHeatmapPages(); | ||
|
|
||
| console.info(pagesData); |
Contributor
There was a problem hiding this comment.
Remove debug logging before merge.
console.info(pagesData) will leak data to the console in production.
🧹 Proposed fix
- console.info(pagesData);📝 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.
Suggested change
| console.info(pagesData); |
🤖 Prompt for AI Agents
In `@client/src/app/`[site]/heatmaps/page.tsx at line 24, Remove the debug
console.info call that prints pagesData in page.tsx: locate the
console.info(pagesData) statement and delete it (or replace it with a
conditional debug-only logger gated by NODE_ENV === 'development' if you need
runtime debug). Ensure no other direct console logging of pagesData remains in
the component or related functions (search for pagesData usage) before merging.
Comment on lines
+46
to
+61
| // Find max value for normalization | ||
| const maxValue = Math.max(...points.map((p) => p.value), 1); | ||
|
|
||
| // Create offscreen canvas for the alpha mask | ||
| const alphaCanvas = document.createElement("canvas"); | ||
| alphaCanvas.width = width; | ||
| alphaCanvas.height = height; | ||
| const alphaCtx = alphaCanvas.getContext("2d"); | ||
| if (!alphaCtx) return; | ||
|
|
||
| // Draw circles for each point on alpha canvas | ||
| points.forEach((point) => { | ||
| // Convert percentage coordinates to pixel coordinates | ||
| const x = (point.x / gridResolution) * width; | ||
| const y = (point.y / gridResolution) * height; | ||
| const intensity = point.value / maxValue; |
Contributor
There was a problem hiding this comment.
Guard against zero gridResolution and avoid large spread in Math.max.
A zero/invalid gridResolution produces Infinity coordinates, and Math.max(...points.map()) can be expensive for large datasets. Clamp the divisor and use a reducer.
♻️ Proposed fix
- const maxValue = Math.max(...points.map((p) => p.value), 1);
+ const maxValue = points.reduce((max, p) => Math.max(max, p.value), 1);
+ const safeGridResolution = Math.max(1, gridResolution);
@@
- const x = (point.x / gridResolution) * width;
- const y = (point.y / gridResolution) * height;
+ const x = (point.x / safeGridResolution) * width;
+ const y = (point.y / safeGridResolution) * height;🤖 Prompt for AI Agents
In `@client/src/components/heatmap/HeatmapCanvas.tsx` around lines 46 - 61, Guard
against zero/invalid gridResolution and avoid spreading a large array when
computing maxValue: clamp gridResolution to a minimum of 1 before converting
coordinates (used in the x/y calculations where gridResolution appears) to
prevent Infinity, and replace Math.max(...points.map((p) => p.value), 1) with a
reducer that computes the maxValue safely (e.g.,
points.reduce((m,p)=>Math.max(m,p.value), 1)) so you don't spread the array;
keep the existing alphaCanvas/alphaCtx handling but ensure downstream intensity
uses the computed maxValue.
Comment on lines
+16
to
+29
| export async function getClickHeatmap(req: FastifyRequest<GetClickHeatmapRequest>, res: FastifyReply) { | ||
| const { pathname, viewportBreakpoint, gridResolution, ...filterParams } = req.query; | ||
| const siteId = Number(req.params.siteId); | ||
|
|
||
| if (!pathname) { | ||
| return res.status(400).send({ error: "pathname is required" }); | ||
| } | ||
|
|
||
| try { | ||
| const result = await clickHeatmapService.getClickHeatmap(siteId, pathname, { | ||
| ...filterParams, | ||
| viewportBreakpoint: viewportBreakpoint || "all", | ||
| gridResolution: gridResolution ? Number(gridResolution) : 100, | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and read the target file
fd getClickHeatmap.ts --type fRepository: rybbit-io/rybbit
Length of output: 176
🏁 Script executed:
# Look for the file structure
find . -name "getClickHeatmap.ts" -type fRepository: rybbit-io/rybbit
Length of output: 103
🏁 Script executed:
# Search for the heatmap service implementation
fd -e ts -e tsx . server/src/api/heatmap/ 2>/dev/null | head -20Repository: rybbit-io/rybbit
Length of output: 175
🏁 Script executed:
cat -n server/src/api/heatmap/getClickHeatmap.tsRepository: rybbit-io/rybbit
Length of output: 1562
🏁 Script executed:
# Look for the service implementation
fd -e ts clickHeatmapService --type fRepository: rybbit-io/rybbit
Length of output: 110
🏁 Script executed:
# Find where GetClickHeatmapRequest is defined
rg "GetClickHeatmapRequest" --type tsRepository: rybbit-io/rybbit
Length of output: 289
🏁 Script executed:
cat -n server/src/services/heatmap/clickHeatmapService.tsRepository: rybbit-io/rybbit
Length of output: 6774
🏁 Script executed:
# Check if there are any validation utilities in the codebase
rg "Number\.isFinite|isInteger" --type ts server/src/ | head -20Repository: rybbit-io/rybbit
Length of output: 278
🏁 Script executed:
# Look for FilterParams definition to understand full context
rg "FilterParams" --type ts -A 5 | head -40Repository: rybbit-io/rybbit
Length of output: 3118
Add input validation for siteId, viewportBreakpoint, and gridResolution to return 400 responses instead of 500s on invalid input.
Line 18 Number(req.params.siteId) can produce NaN, and gridResolution conversion lacks bounds checking. Additionally, viewportBreakpoint has no runtime validation despite being an enum. Invalid values will propagate to ClickHouse and trigger generic 500 errors instead of explicit 400 responses.
Input validation example
const { pathname, viewportBreakpoint, gridResolution, ...filterParams } = req.query;
const siteId = Number(req.params.siteId);
+ if (!Number.isFinite(siteId) || siteId <= 0) {
+ return res.status(400).send({ error: "siteId must be a positive number" });
+ }
+
+ const allowedBreakpoints = new Set(["mobile", "tablet", "desktop", "all"]);
+ if (viewportBreakpoint && !allowedBreakpoints.has(viewportBreakpoint)) {
+ return res.status(400).send({ error: "viewportBreakpoint is invalid" });
+ }
+
+ const parsedGridResolution = gridResolution ? Number(gridResolution) : 100;
+ if (
+ !Number.isInteger(parsedGridResolution) ||
+ parsedGridResolution <= 0 ||
+ parsedGridResolution > 65535
+ ) {
+ return res.status(400).send({ error: "gridResolution must be an integer between 1 and 65535" });
+ }🤖 Prompt for AI Agents
In `@server/src/api/heatmap/getClickHeatmap.ts` around lines 16 - 29, Validate
inputs in getClickHeatmap before calling clickHeatmapService: ensure siteId
(derived from Number(req.params.siteId)) is a valid positive integer (check
isNaN and >0) and return res.status(400) with a clear message if invalid;
validate viewportBreakpoint from req.query against the allowed enum values
(explicitly list acceptable strings) and return 400 if it is not one of them;
coerce gridResolution to a Number, ensure it is a finite integer within
acceptable bounds (e.g., >0 and <= a sensible max like 1000) and return 400 for
out-of-range or non-integer values; only after these checks call
clickHeatmapService.getClickHeatmap with the validated/normalized values.
Comment on lines
+14
to
+22
| export async function getHeatmapPages(req: FastifyRequest<GetHeatmapPagesRequest>, res: FastifyReply) { | ||
| const { limit, ...filterParams } = req.query; | ||
| const siteId = Number(req.params.siteId); | ||
|
|
||
| try { | ||
| const pages = await clickHeatmapService.getHeatmapPages(siteId, { | ||
| ...filterParams, | ||
| limit: limit ? Number(limit) : 100, | ||
| }); |
Contributor
There was a problem hiding this comment.
Add input validation for siteId and limit before service call.
Number(req.params.siteId) and Number(limit) can produce NaN or invalid values (negative/zero). The service expects valid positive integers. Validate and return 400 if either is invalid:
Input validation example
const { limit, ...filterParams } = req.query;
const siteId = Number(req.params.siteId);
+
+ if (!Number.isFinite(siteId) || siteId <= 0) {
+ return res.status(400).send({ error: "siteId must be a positive number" });
+ }
+
+ const parsedLimit = limit ? Number(limit) : 100;
+ if (!Number.isInteger(parsedLimit) || parsedLimit <= 0) {
+ return res.status(400).send({ error: "limit must be a positive integer" });
+ }
@@
- limit: limit ? Number(limit) : 100,
+ limit: parsedLimit,
});🤖 Prompt for AI Agents
In `@server/src/api/heatmap/getHeatmapPages.ts` around lines 14 - 22, Validate
req.params.siteId and req.query.limit before calling
clickHeatmapService.getHeatmapPages: parse siteId from req.params.siteId and
ensure Number.isInteger(siteId) && siteId > 0, parse limit from req.query.limit
(fall back to 100) and ensure Number.isInteger(limit) && limit > 0; if either
check fails, return a 400 response from getHeatmapPages with a clear error
message (use res.code(400).send(...)) instead of calling
clickHeatmapService.getHeatmapPages.
Comment on lines
+52
to
+128
| async getClickHeatmap( | ||
| siteId: number, | ||
| pathname: string, | ||
| options: FilterParams<{ | ||
| viewportBreakpoint?: ViewportBreakpoint; | ||
| gridResolution?: number; // Number of grid cells (default 100 = 1% resolution) | ||
| }> | ||
| ): Promise<ClickHeatmapResult> { | ||
| const { viewportBreakpoint = "all", gridResolution = 100 } = options; | ||
|
|
||
| // Build time statement for filtering | ||
| const timeStatement = getTimeStatement(options).replace(/timestamp/g, "src.timestamp"); | ||
| const viewportCondition = getViewportCondition(viewportBreakpoint).replace(/viewport_width/g, "src.viewport_width"); | ||
|
|
||
| // Build pathname pattern for LIKE query | ||
| // Remove trailing slashes and handle exact match | ||
| const cleanPathname = pathname.replace(/\/+$/, "") || "/"; | ||
| const pathnamePattern = cleanPathname === "/" ? "/" : `%${cleanPathname}%`; | ||
|
|
||
| const query = ` | ||
| SELECT | ||
| ROUND(src.x / src.viewport_width * {gridResolution:UInt16}, 0) as x, | ||
| ROUND(src.y / src.viewport_height * {gridResolution:UInt16}, 0) as y, | ||
| COUNT(*) as value | ||
| FROM session_replay_clicks src | ||
| INNER JOIN session_replay_metadata srm ON src.session_id = srm.session_id AND src.site_id = srm.site_id | ||
| WHERE src.site_id = {siteId:UInt16} | ||
| AND src.viewport_width > 0 | ||
| AND src.viewport_height > 0 | ||
| AND src.x >= 0 AND src.y >= 0 | ||
| AND src.x <= src.viewport_width | ||
| AND src.y <= src.viewport_height | ||
| AND srm.page_url LIKE {pathnamePattern:String} | ||
| ${viewportCondition} | ||
| ${timeStatement} | ||
| GROUP BY x, y | ||
| HAVING value >= 1 | ||
| ORDER BY value DESC | ||
| LIMIT 10000 | ||
| `; | ||
|
|
||
| const statsQuery = ` | ||
| SELECT | ||
| COUNT(*) as totalClicks, | ||
| COUNT(DISTINCT src.session_id) as uniqueSessions | ||
| FROM session_replay_clicks src | ||
| INNER JOIN session_replay_metadata srm ON src.session_id = srm.session_id AND src.site_id = srm.site_id | ||
| WHERE src.site_id = {siteId:UInt16} | ||
| AND src.viewport_width > 0 | ||
| AND src.viewport_height > 0 | ||
| AND srm.page_url LIKE {pathnamePattern:String} | ||
| ${viewportCondition} | ||
| ${timeStatement} | ||
| `; | ||
|
|
||
| const [pointsResult, statsResult] = await Promise.all([ | ||
| clickhouse.query({ | ||
| query, | ||
| query_params: { siteId, pathnamePattern, gridResolution }, | ||
| format: "JSONEachRow", | ||
| }), | ||
| clickhouse.query({ | ||
| query: statsQuery, | ||
| query_params: { siteId, pathnamePattern }, | ||
| format: "JSONEachRow", | ||
| }), | ||
| ]); | ||
|
|
||
| const points = await processResults<HeatmapDataPoint>(pointsResult); | ||
| const stats = await processResults<{ totalClicks: number; uniqueSessions: number }>(statsResult); | ||
|
|
||
| return { | ||
| points, | ||
| totalClicks: stats[0]?.totalClicks ?? 0, | ||
| uniqueSessions: stats[0]?.uniqueSessions ?? 0, | ||
| }; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd --type f -name "clickHeatmapService.ts"Repository: rybbit-io/rybbit
Length of output: 176
🏁 Script executed:
# Get the full content of the file to understand context
find . -type f -name "clickHeatmapService.ts" -exec wc -l {} \;Repository: rybbit-io/rybbit
Length of output: 116
🏁 Script executed:
# Explore the server directory structure
git ls-files server/src/services/heatmap/ | head -20Repository: rybbit-io/rybbit
Length of output: 110
🏁 Script executed:
cat -n server/src/services/heatmap/clickHeatmapService.tsRepository: rybbit-io/rybbit
Length of output: 6774
🏁 Script executed:
# Search for page_url usage and schema definitions
rg "page_url" --type ts -B 2 -A 2Repository: rybbit-io/rybbit
Length of output: 5388
🏁 Script executed:
# Search for path() function usage in ClickHouse queries
rg "path\(" --type ts -B 2 -A 2Repository: rybbit-io/rybbit
Length of output: 474
🏁 Script executed:
# Search for similar filtering patterns and understand how pages are handled elsewhere
rg "srm\.page_url|page_url.*LIKE|path\(.*page" --type ts -B 2 -A 2Repository: rybbit-io/rybbit
Length of output: 1360
Use path() function for consistent and accurate pathname filtering.
The current LIKE pattern matching on the full URL (srm.page_url LIKE {pathnamePattern:String}) is incorrect. Since page_url contains full URLs (e.g., https://example.com/path), filtering with LIKE "/" won't match, and LIKE "%/about%" matches unrelated paths like /about-us. The getHeatmapPages() method already correctly uses path(srm.page_url) for filtering—apply the same approach here for exact path matching:
🔧 Suggested query change
- const pathnamePattern = cleanPathname === "/" ? "/" : `%${cleanPathname}%`;
+ const pathnameFilter = cleanPathname;
@@
- AND srm.page_url LIKE {pathnamePattern:String}
+ AND path(srm.page_url) = {pathnameFilter:String}
@@
- AND srm.page_url LIKE {pathnamePattern:String}
+ AND path(srm.page_url) = {pathnameFilter:String}
@@
- query_params: { siteId, pathnamePattern, gridResolution },
+ query_params: { siteId, pathnameFilter, gridResolution },
@@
- query_params: { siteId, pathnamePattern },
+ query_params: { siteId, pathnameFilter },📝 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.
Suggested change
| async getClickHeatmap( | |
| siteId: number, | |
| pathname: string, | |
| options: FilterParams<{ | |
| viewportBreakpoint?: ViewportBreakpoint; | |
| gridResolution?: number; // Number of grid cells (default 100 = 1% resolution) | |
| }> | |
| ): Promise<ClickHeatmapResult> { | |
| const { viewportBreakpoint = "all", gridResolution = 100 } = options; | |
| // Build time statement for filtering | |
| const timeStatement = getTimeStatement(options).replace(/timestamp/g, "src.timestamp"); | |
| const viewportCondition = getViewportCondition(viewportBreakpoint).replace(/viewport_width/g, "src.viewport_width"); | |
| // Build pathname pattern for LIKE query | |
| // Remove trailing slashes and handle exact match | |
| const cleanPathname = pathname.replace(/\/+$/, "") || "/"; | |
| const pathnamePattern = cleanPathname === "/" ? "/" : `%${cleanPathname}%`; | |
| const query = ` | |
| SELECT | |
| ROUND(src.x / src.viewport_width * {gridResolution:UInt16}, 0) as x, | |
| ROUND(src.y / src.viewport_height * {gridResolution:UInt16}, 0) as y, | |
| COUNT(*) as value | |
| FROM session_replay_clicks src | |
| INNER JOIN session_replay_metadata srm ON src.session_id = srm.session_id AND src.site_id = srm.site_id | |
| WHERE src.site_id = {siteId:UInt16} | |
| AND src.viewport_width > 0 | |
| AND src.viewport_height > 0 | |
| AND src.x >= 0 AND src.y >= 0 | |
| AND src.x <= src.viewport_width | |
| AND src.y <= src.viewport_height | |
| AND srm.page_url LIKE {pathnamePattern:String} | |
| ${viewportCondition} | |
| ${timeStatement} | |
| GROUP BY x, y | |
| HAVING value >= 1 | |
| ORDER BY value DESC | |
| LIMIT 10000 | |
| `; | |
| const statsQuery = ` | |
| SELECT | |
| COUNT(*) as totalClicks, | |
| COUNT(DISTINCT src.session_id) as uniqueSessions | |
| FROM session_replay_clicks src | |
| INNER JOIN session_replay_metadata srm ON src.session_id = srm.session_id AND src.site_id = srm.site_id | |
| WHERE src.site_id = {siteId:UInt16} | |
| AND src.viewport_width > 0 | |
| AND src.viewport_height > 0 | |
| AND srm.page_url LIKE {pathnamePattern:String} | |
| ${viewportCondition} | |
| ${timeStatement} | |
| `; | |
| const [pointsResult, statsResult] = await Promise.all([ | |
| clickhouse.query({ | |
| query, | |
| query_params: { siteId, pathnamePattern, gridResolution }, | |
| format: "JSONEachRow", | |
| }), | |
| clickhouse.query({ | |
| query: statsQuery, | |
| query_params: { siteId, pathnamePattern }, | |
| format: "JSONEachRow", | |
| }), | |
| ]); | |
| const points = await processResults<HeatmapDataPoint>(pointsResult); | |
| const stats = await processResults<{ totalClicks: number; uniqueSessions: number }>(statsResult); | |
| return { | |
| points, | |
| totalClicks: stats[0]?.totalClicks ?? 0, | |
| uniqueSessions: stats[0]?.uniqueSessions ?? 0, | |
| }; | |
| } | |
| async getClickHeatmap( | |
| siteId: number, | |
| pathname: string, | |
| options: FilterParams<{ | |
| viewportBreakpoint?: ViewportBreakpoint; | |
| gridResolution?: number; // Number of grid cells (default 100 = 1% resolution) | |
| }> | |
| ): Promise<ClickHeatmapResult> { | |
| const { viewportBreakpoint = "all", gridResolution = 100 } = options; | |
| // Build time statement for filtering | |
| const timeStatement = getTimeStatement(options).replace(/timestamp/g, "src.timestamp"); | |
| const viewportCondition = getViewportCondition(viewportBreakpoint).replace(/viewport_width/g, "src.viewport_width"); | |
| // Build pathname pattern for LIKE query | |
| // Remove trailing slashes and handle exact match | |
| const cleanPathname = pathname.replace(/\/+$/, "") || "/"; | |
| const pathnameFilter = cleanPathname; | |
| const query = ` | |
| SELECT | |
| ROUND(src.x / src.viewport_width * {gridResolution:UInt16}, 0) as x, | |
| ROUND(src.y / src.viewport_height * {gridResolution:UInt16}, 0) as y, | |
| COUNT(*) as value | |
| FROM session_replay_clicks src | |
| INNER JOIN session_replay_metadata srm ON src.session_id = srm.session_id AND src.site_id = srm.site_id | |
| WHERE src.site_id = {siteId:UInt16} | |
| AND src.viewport_width > 0 | |
| AND src.viewport_height > 0 | |
| AND src.x >= 0 AND src.y >= 0 | |
| AND src.x <= src.viewport_width | |
| AND src.y <= src.viewport_height | |
| AND path(srm.page_url) = {pathnameFilter:String} | |
| ${viewportCondition} | |
| ${timeStatement} | |
| GROUP BY x, y | |
| HAVING value >= 1 | |
| ORDER BY value DESC | |
| LIMIT 10000 | |
| `; | |
| const statsQuery = ` | |
| SELECT | |
| COUNT(*) as totalClicks, | |
| COUNT(DISTINCT src.session_id) as uniqueSessions | |
| FROM session_replay_clicks src | |
| INNER JOIN session_replay_metadata srm ON src.session_id = srm.session_id AND src.site_id = srm.site_id | |
| WHERE src.site_id = {siteId:UInt16} | |
| AND src.viewport_width > 0 | |
| AND src.viewport_height > 0 | |
| AND path(srm.page_url) = {pathnameFilter:String} | |
| ${viewportCondition} | |
| ${timeStatement} | |
| `; | |
| const [pointsResult, statsResult] = await Promise.all([ | |
| clickhouse.query({ | |
| query, | |
| query_params: { siteId, pathnameFilter, gridResolution }, | |
| format: "JSONEachRow", | |
| }), | |
| clickhouse.query({ | |
| query: statsQuery, | |
| query_params: { siteId, pathnameFilter }, | |
| format: "JSONEachRow", | |
| }), | |
| ]); | |
| const points = await processResults<HeatmapDataPoint>(pointsResult); | |
| const stats = await processResults<{ totalClicks: number; uniqueSessions: number }>(statsResult); | |
| return { | |
| points, | |
| totalClicks: stats[0]?.totalClicks ?? 0, | |
| uniqueSessions: stats[0]?.uniqueSessions ?? 0, | |
| }; | |
| } |
🤖 Prompt for AI Agents
In `@server/src/services/heatmap/clickHeatmapService.ts` around lines 52 - 128, In
getClickHeatmap replace the current srm.page_url LIKE {pathnamePattern:String}
filtering in both query and statsQuery with path(srm.page_url) =
{pathname:String} (so use ClickHouse's path() on srm.page_url for exact-path
matching), adjust the parameter you pass in query_params from pathnamePattern to
pathname (set to cleanPathname, with "/" for root), and remove/stop using the
LIKE-based pathnamePattern logic; update references in clickhouse.query calls
and query_params accordingly in the getClickHeatmap function.
Comment on lines
+134
to
+169
| async getHeatmapPages( | ||
| siteId: number, | ||
| options: FilterParams<{ | ||
| limit?: number; | ||
| }> | ||
| ): Promise<HeatmapPage[]> { | ||
| const { limit = 100 } = options; | ||
|
|
||
| const timeStatement = getTimeStatement(options).replace(/timestamp/g, "sre.timestamp"); | ||
|
|
||
| // Query the dedicated session_replay_clicks table | ||
| // This table is populated during ingestion with click coordinates extracted from rrweb events | ||
| const query = ` | ||
| SELECT | ||
| path(srm.page_url) as pathname, | ||
| COUNT(*) as clickCount, | ||
| COUNT(DISTINCT src.session_id) as sessionCount | ||
| FROM session_replay_clicks src | ||
| INNER JOIN session_replay_metadata srm ON src.session_id = srm.session_id AND src.site_id = srm.site_id | ||
| WHERE src.site_id = {siteId:UInt16} | ||
| ${timeStatement} | ||
| GROUP BY pathname | ||
| ORDER BY clickCount DESC | ||
| LIMIT {limit:UInt32} | ||
| `; | ||
|
|
||
| const result = await clickhouse.query({ | ||
| query, | ||
| query_params: { siteId, limit }, | ||
| format: "JSONEachRow", | ||
| }); | ||
|
|
||
| const pages = await processResults<HeatmapPage>(result); | ||
|
|
||
| return pages; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the getTimeStatement function definition
rg "getTimeStatement" --type ts --type tsx -B 2 -A 5Repository: rybbit-io/rybbit
Length of output: 87
🏁 Script executed:
# Also check the file directly to see imports and context
fd "clickHeatmapService.ts" | head -1 | xargs cat -nRepository: rybbit-io/rybbit
Length of output: 6774
Fix incorrect table alias in time filter (sre → src).
At line 142, the query uses alias src for session_replay_clicks, but the time filter replaces with sre.timestamp, which doesn't exist. This will cause query failures. Change to src.timestamp to match the table alias.
🛠️ Fix alias
- const timeStatement = getTimeStatement(options).replace(/timestamp/g, "sre.timestamp");
+ const timeStatement = getTimeStatement(options).replace(/timestamp/g, "src.timestamp");📝 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.
Suggested change
| async getHeatmapPages( | |
| siteId: number, | |
| options: FilterParams<{ | |
| limit?: number; | |
| }> | |
| ): Promise<HeatmapPage[]> { | |
| const { limit = 100 } = options; | |
| const timeStatement = getTimeStatement(options).replace(/timestamp/g, "sre.timestamp"); | |
| // Query the dedicated session_replay_clicks table | |
| // This table is populated during ingestion with click coordinates extracted from rrweb events | |
| const query = ` | |
| SELECT | |
| path(srm.page_url) as pathname, | |
| COUNT(*) as clickCount, | |
| COUNT(DISTINCT src.session_id) as sessionCount | |
| FROM session_replay_clicks src | |
| INNER JOIN session_replay_metadata srm ON src.session_id = srm.session_id AND src.site_id = srm.site_id | |
| WHERE src.site_id = {siteId:UInt16} | |
| ${timeStatement} | |
| GROUP BY pathname | |
| ORDER BY clickCount DESC | |
| LIMIT {limit:UInt32} | |
| `; | |
| const result = await clickhouse.query({ | |
| query, | |
| query_params: { siteId, limit }, | |
| format: "JSONEachRow", | |
| }); | |
| const pages = await processResults<HeatmapPage>(result); | |
| return pages; | |
| } | |
| async getHeatmapPages( | |
| siteId: number, | |
| options: FilterParams<{ | |
| limit?: number; | |
| }> | |
| ): Promise<HeatmapPage[]> { | |
| const { limit = 100 } = options; | |
| const timeStatement = getTimeStatement(options).replace(/timestamp/g, "src.timestamp"); | |
| // Query the dedicated session_replay_clicks table | |
| // This table is populated during ingestion with click coordinates extracted from rrweb events | |
| const query = ` | |
| SELECT | |
| path(srm.page_url) as pathname, | |
| COUNT(*) as clickCount, | |
| COUNT(DISTINCT src.session_id) as sessionCount | |
| FROM session_replay_clicks src | |
| INNER JOIN session_replay_metadata srm ON src.session_id = srm.session_id AND src.site_id = srm.site_id | |
| WHERE src.site_id = {siteId:UInt16} | |
| ${timeStatement} | |
| GROUP BY pathname | |
| ORDER BY clickCount DESC | |
| LIMIT {limit:UInt32} | |
| `; | |
| const result = await clickhouse.query({ | |
| query, | |
| query_params: { siteId, limit }, | |
| format: "JSONEachRow", | |
| }); | |
| const pages = await processResults<HeatmapPage>(result); | |
| return pages; | |
| } |
🤖 Prompt for AI Agents
In `@server/src/services/heatmap/clickHeatmapService.ts` around lines 134 - 169,
In getHeatmapPages, the generated timeStatement incorrectly references
"sre.timestamp" while the query aliases session_replay_clicks as src; update the
replacement so getTimeStatement(options).replace(/timestamp/g, "src.timestamp")
(or otherwise ensure timeStatement uses the src alias) so the WHERE clause
matches the src table alias used in the query's WHERE clause and joins.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.