Lite dashboard 2#1011
Merged
Merged
Conversation
- Added DEPLOYMENT environment variable to docker-compose and client Dockerfile for better deployment management. - Introduced CLUSTER_WORKERS variable in docker-compose to control the number of cluster workers, defaulting to 4. - Updated constants in client and server to include DEPLOYMENT for consistent access across the application.
- Changed Dockerfile paths in docker-compose.yml to use relative paths for better clarity and consistency across services.
- Added port mapping for PostgreSQL service to allow external connections on port 5432.
- Modified the conditional rendering in AdminLink to account for both IS_CLOUD and DEPLOYMENT variables, ensuring proper access control for admin features.
- Introduced endpoints for retrieving admin sites and organizations, as well as a service event count. - Added a public telemetry collection endpoint to enhance monitoring capabilities.
- Changed data types for pageviews and events from UInt32 to UInt64 to accommodate larger values.
…ormance - Reorganized SQL queries in getMetricLite, getOverviewBucketedLite, and getOverviewLite to improve readability and performance. - Introduced subqueries to handle aggregations more effectively, ensuring compliance with ClickHouse's limitations on nested aggregates. - Updated variable names for clarity, enhancing maintainability of the code.
- Changed the ORDER BY clause in getMetricLite from pageviews to count for both pathname and country queries, ensuring consistent metric sorting across analytics endpoints.
…tedLite and getOverviewLite - Refactored SQL queries in getOverviewBucketedLite and getOverviewLite to utilize a refreshable materialized view for session metrics, enhancing performance and reducing data retrieval complexity. - Introduced helper functions for building hour and day bucket queries, improving code readability and maintainability. - Updated time statement logic to align with session hour granularity, ensuring accurate metric calculations across different time buckets.
…verviewLite for improved aggregation handling - Refactored SQL queries to utilize subqueries for aggregations, enhancing clarity and compliance with ClickHouse's aggregation rules. - Updated the outer SELECT statements to directly reference computed values, avoiding aliasing issues that could lead to errors. - Improved comments for better understanding of the query structure and logic.
…sections - Added an Expand button to both CountriesLite and PagesLite components for improved user interaction. - Updated layout to include a flexbox for better alignment of titles and buttons. - Integrated the Button component from the UI library for consistent styling.
- Added new keys for API endpoint and filter-related messages in Czech, German, English, Spanish, French, Italian, and Japanese localization files. - Removed outdated keys to streamline the localization content. - Enhanced user experience by providing clearer instructions and error messages related to API interactions.
- Updated the Chart component in MainSectionLite to include the chartXMax prop, enhancing its configurability for rendering data.
- Updated the analytics API to support device type in the lite metric fetching functionality. - Modified relevant components and hooks to accommodate the new dimension, ensuring consistent data retrieval. - Enhanced the ClickHouse schema and backfill scripts to include device type materialized views for improved analytics capabilities.
- Introduced a new metric for Total Time in the OverviewLite component, calculated from session duration and sessions. - Updated localization files to include the new metric in multiple languages, ensuring consistent user experience across different locales. - Enhanced the Stat component to handle the new metric and adjusted layout for proper display.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds lite analytics backed by ClickHouse MVs and server endpoints, threads a lite flag through client hooks/components/pages (including rollup), introduces mobile site type with RN SDK UI/validation, adds org user creation, updates env/config/docker, and refreshes i18n/docs strings. ChangesLite Analytics Flow
Mobile Site Type and Settings
React Native SDK
Anonymous ID in tracker
Create User in Organization
Client and Docs i18n
Small formatting/misc
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![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: 2
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
client/src/components/SiteSettings/GeneralTab.tsx (1)
121-136:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep the failure toast consistent for mobile sites.
This branch now presents the field as an app identifier for mobile sites, but the error path still always says "Failed to update domain", which gives contradictory feedback after a rejected mobile edit.
💡 Suggested fix
} catch (error) { console.error("Error changing domain:", error); - toast.error(t("Failed to update domain")); + toast.error(isMobileSite ? t("Failed to update app identifier") : t("Failed to update domain")); } finally {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/src/components/SiteSettings/GeneralTab.tsx` around lines 121 - 136, In handleDomainChange, the catch branch always shows "Failed to update domain" even for mobile sites; update the toast.error call inside the catch to mirror the success branch by conditionally using the mobile message when isMobileSite is true (e.g., use t("Failed to update app identifier") for mobile or t("Failed to update domain") otherwise) so the failure toast matches the field label; adjust the toast.error invocation accordingly and keep the existing console.error call.client/src/api/analytics/hooks/useGetMetric.ts (2)
121-126:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the
prevSiteindex used byplaceholderData.With the current key shape,
siteis at index 3, not 2. As written, placeholder reuse always misses because it compares the current site to the previoustimeobject.💡 Suggested fix
- const [, , prevSite] = prevQueryKey; + const [, , , prevSite] = prevQueryKey;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/src/api/analytics/hooks/useGetMetric.ts` around lines 121 - 126, The placeholderData logic in useGetMetric.ts is grabbing the wrong element from the previous query key: change the destructuring so prevSite comes from index 3 (not 2) — e.g., extract prevQueryKey[3] or use [, , , prevSite] = prevQueryKey — then compare that prevSite to the current site so placeholder reuse works correctly; update any related variable names (prevQueryKey, prevSite, placeholderData) accordingly.
154-176:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the effective filter set in the infinite-query key too.
useInfiniteMetrichas the same lite-mode key mismatch: the request ignores filters, but the key still changes with filter UI state. That will cause unnecessary invalidations/refetches for the same lite dataset.💡 Suggested fix
queryKey: [ parameter, customTime, time, site, - filters, + combinedFilters, limit, - additionalFilters, - customFilters, "infinite-metric", timezone, lite, ],As per coding guidelines, "Keep React Query keys as descriptive arrays and include every input that affects the response."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/src/api/analytics/hooks/useGetMetric.ts` around lines 154 - 176, The query key currently includes raw filter UI state (filters, additionalFilters, customFilters) while the request uses the effective filters computed into combinedFilters; update the useInfiniteQuery queryKey in useGetMetric (the call to useInfiniteQuery) to include the effective filter set (combinedFilters) instead of the separate filter arrays so the key matches the actual request inputs (preserving lite behavior) and prevents unnecessary refetches; ensure any other inputs already in the key (parameter, customTime, time, site, limit, timezone, lite) remain unchanged.client/src/app/components/AddSite.tsx (1)
104-137:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the remaining “websites” copy to match the new site types.
These tooltips still talk about adding “websites”, which is now incorrect when the user is adding a React Native app.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/src/app/components/AddSite.tsx` around lines 104 - 137, In the AddSite component update the copy that says "websites" to the neutral site type wording (e.g., "sites" or your canonical site type name) so the tooltips and button title are correct for React Native apps; specifically change the Button title strings (currently t("Upgrade to Pro to add more websites")) and the TooltipContent texts (both t("You need to be on an active subscription to add websites") and the limit message t("You have reached the limit of {limit} websites. Upgrade to add more websites", { limit: String(siteLimit) })) to use the new wording, keeping the same translation interpolation for siteLimit and using the same t(...) function calls in the AddSite component where subscription, IS_CLOUD, isOverSiteLimit, trigger, and siteLimit are used.server/src/api/sites/updateSiteConfig.ts (1)
205-209:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winConflict text is now wrong for mobile sites.
A duplicate mobile app identifier will still hit this branch and return
"Domain already in use", which is misleading in the new mobile flow. Please make this message type-aware or neutral, e.g. domain/app identifier.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/api/sites/updateSiteConfig.ts` around lines 205 - 209, The duplicate-key branch in updateSiteConfig (the block using String(error).includes("duplicate key value violates unique constraint") that calls reply.status(409).send) returns "Domain already in use" which is wrong for mobile sites; change this to a neutral or type-aware message: either return a neutral message like "Domain or app identifier already in use" or inspect the incoming payload (e.g., request.body.siteType or presence of mobile identifier fields) and return "App identifier already in use" for mobile and "Domain already in use" for web, keeping the response format and status code unchanged.client/messages/uk.json (1)
1265-1304:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftWide blank-string block in Ukrainian locale will leave embed/tracking UI unlabeled
Many changed keys in this section are now empty strings, including core embed and tracking copy (starting around Line 1265 through Line 1304, plus Line 1420/1422/1430). This is likely to produce unlabeled controls and missing explanatory text in production.
Also applies to: 1306-1307, 1328-1330, 1383-1385, 1420-1431
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/messages/uk.json` around lines 1265 - 1304, This locale file shows many message keys set to empty strings (e.g., "+CwN9C", "dmW5bt", "PkGCG1", ..., "2YpSsF") which will render unlabeled UI; restore proper Ukrainian translations for each empty key (or apply a fallback to the English string) so embed/tracking controls are labeled—inspect the nearby known key "JfG49w" ("Відкрито") as a translation example and update the same key groups flagged (around the blocks you changed and also keys at 1306-1307, 1328-1330, 1383-1385, 1420-1431) to non-empty Ukrainian strings or fallbacks to avoid missing UI text.
🟠 Major comments (26)
server/src/scripts/backfillLiteDashboardMVs.ts-133-137 (1)
133-137:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject duplicate entries in
--tables.
--tables sessions,sessionscurrently backfills the same target twice, which will double-count data in a single run. That is especially risky here because this script already documents the targets as non-idempotent on re-insert.Suggested fix
else if (a === "--tables") { const list = args[++i]?.split(",").map(s => s.trim()) ?? []; + const unique = [...new Set(list)]; const invalid = list.filter(t => !ALL_TABLES.includes(t as TableKey)); if (invalid.length) throw new Error(`Unknown table(s): ${invalid.join(", ")}`); - tables = list as TableKey[]; + if (unique.length !== list.length) { + throw new Error("--tables cannot contain duplicates"); + } + tables = unique as TableKey[]; } else if (a === "--help" || a === "-h") {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/scripts/backfillLiteDashboardMVs.ts` around lines 133 - 137, The --tables parsing currently allows duplicate table names (e.g., sessions,sessions) which leads to double-processing; update the branch handling a === "--tables" (the parsing that builds list and sets tables) to detect duplicate entries after trimming and before assigning tables: compute duplicates (elements whose first index !== last index or via a Set check), if any duplicates are found throw a new Error identifying the duplicate table names (similar format to the existing unknown table error), otherwise continue to validate against ALL_TABLES and then assign tables = list as TableKey[]; ensure the duplicate check runs before final assignment so the script rejects duplicate input early.server/src/services/userId/userIdService.ts-70-74 (1)
70-74:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't silently disable salting when config lookup fails.
siteConfig.getConfig(siteId)returnsundefinedon lookup errors, and this branch then hashes with"". A transient config failure would therefore generate unsalted, stable IDs for sites that enabledsaltUserIds, which weakens the privacy behavior instead of failing closed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/services/userId/userIdService.ts` around lines 70 - 74, The code in generateUserIdFromClientId currently treats a failed siteConfig.getConfig(siteId) as if saltUserIds is false and uses an empty salt; instead detect when config is undefined and fail closed by throwing or returning a rejected Promise so you don't fall back to an unsalted hash. Update generateUserIdFromClientId to check if config === undefined (from siteConfig.getConfig) and if so propagate an error (or return a clear failure) rather than using "" for salt; when config exists, continue using config.saltUserIds and this.getDailySalt() to compute the SHA-256 and substring as before, ensuring callers handle the thrown/rejected error.server/src/lib/siteConfig.ts-11-11 (1)
11-11:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPersist the new
typefield on site creation.
SiteConfigDatanow requirestype, butaddSite()in this file still doesn't write it tosites. New mobile sites created through this helper will fall back to"web"when they are read back, which breaks the new site-type contract.Suggested fix
await db.insert(sites).values({ id: config.id, name: "", // This would need to be provided + type: config.type, domain: config.domain, public: config.public, saltUserIds: config.saltUserIds,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/lib/siteConfig.ts` at line 11, The SiteConfigData type now requires the new `type` field but `addSite()` is not persisting it—update the `addSite` implementation to include the incoming SiteConfigData.type when inserting into the `sites` store/row so new mobile sites are saved with their correct type; locate the `addSite` function in this file and ensure the insert/object written to `sites` contains the `type` property (from the `SiteConfigData` parameter) rather than defaulting to `"web"`.server/src/db/clickhouse/clickhouse.ts-306-324 (1)
306-324:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftFix nondeterministic session-rollup fields in
AggregatingMergeTree
server/src/db/clickhouse/clickhouse.ts(sessions rollup around lines 306-355):sessions_mvcomputesargMin(pathname, timestamp)forentry_pathname(andany(...)forcountry,hostname, etc.) per inserted batch, butsessions_mv_targetstores these as plain scalar columns (String/LowCardinality(...)). WhenAggregatingMergeTreemerges multiple rows for the same(site_id, session_id), these scalars have no deterministic merge behavior, so values can drift (withentry_pathnamebeing the landing-page casualty). Store these fields asAggregateFunction/SimpleAggregateFunctionwith corresponding mergeable states (e.g.,argMinState) or switch to a recomputation/refresh approach instead of streaming MV.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/db/clickhouse/clickhouse.ts` around lines 306 - 324, The sessions_mv_target table currently stores nondeterministic scalar columns (e.g., entry_pathname, country, hostname) derived from per-batch aggregates in sessions_mv and then merged by AggregatingMergeTree, causing drift; change those scalar columns to appropriate aggregate-state types (e.g., use AggregateFunction(argMin, String, DateTime) or argMinState(String, DateTime) / SimpleAggregateFunction-like state types for entry_pathname and AggregateFunction/LowCardinality states for country/hostname) so merges use deterministic mergeable states (or alternatively adopt a recompute/refresh approach instead of streaming MV); update the CREATE TABLE for sessions_mv_target to replace plain String/LowCardinality(...) columns with the corresponding AggregateFunction/State types that match the argMin/any semantics used in sessions_mv (reference sessions_mv_target, sessions_mv, AggregatingMergeTree, entry_pathname, argMin, country, hostname).server/src/api/analytics/lite/getOverviewLite.ts-16-25 (1)
16-25:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
siteIdand query params before building the query.This handler trusts route/query input at runtime. A bad
siteIdbecomesNaNhere and will fall through to ClickHouse as a server error instead of returning a clean 4xx response.As per coding guidelines,
server/src/api/**/*.ts: Validate untrusted request bodies and query params with Zod before using them in route handlers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/api/analytics/lite/getOverviewLite.ts` around lines 16 - 25, The handler getOverviewLite currently converts req.params.siteId to Number and uses req.query without validation, allowing NaN or malformed queries to reach ClickHouse; add Zod schemas to validate the route params (siteId as a numeric/stringable integer) and the Querystring shape used by getLiteTimeStatement before any conversion or query construction, then parse/transform the validated values (e.g., convert siteId to number) and return a 4xx Fastify reply (bad request) when validation fails; locate getOverviewLite and the call to getLiteTimeStatement to insert this validation and error handling.server/src/api/analytics/lite/getMetricLite.ts-30-33 (1)
30-33:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClamp and validate these inputs before interpolating the query.
Math.min(Number(req.query.limit) || 100, 500)still allows negative limits, andsiteIdcan becomeNaN. Both cases turn malformed client input into backend/query failures.As per coding guidelines,
server/src/api/**/*.ts: Validate untrusted request bodies and query params with Zod before using them in route handlers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/api/analytics/lite/getMetricLite.ts` around lines 30 - 33, Validate and clamp request inputs using Zod in getMetricLite.ts: create a Zod schema for req.params.siteId (coerce to number/integer), req.query.limit (coerce to number, default 100, clamp between 1 and 500) and req.query.parameter (validate allowed strings), then replace direct uses of Number(req.params.siteId), req.query.limit and req.query.parameter with the parsed/validated values; ensure you handle schema.parse/ safeParse errors by returning a 400 response before calling getLiteTimeStatement or interpolating into queries so site is never NaN and limit is never negative/out-of-range.client/src/app/rollup/components/sections/PagesLite.tsx-10-14 (1)
10-14:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThis rollup conflates unrelated pages across sites.
The inline note here describes a real data bug: grouping only by
pathnamemeans every selected site's/about,/, etc. are merged into the same bucket. That makes the section misleading for multi-site rollups.A safer interim behavior would be to hide this section when
siteIds.length > 1until the lite endpoint/materialized view can include a hostname/site dimension.Also applies to: 31-41
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/src/app/rollup/components/sections/PagesLite.tsx` around lines 10 - 14, PagesLite currently groups metrics only by pathname and therefore conflates pages across multiple sites; update the PagesLite component (function PagesLite and its siteIds prop) to not render the section when multiple sites are selected by checking siteIds.length > 1 and returning null (or an explanatory placeholder) in that case, so the section is hidden for multi-site rollups until the lite endpoint/materialized view includes a hostname/site dimension; apply the same guard where similar logic is present in the related block referenced (the other PagesLite usage around the same component).docker-compose.yml-65-66 (1)
65-66:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid exposing Postgres on all interfaces by default.
"5432:5432"publishes the database externally on every host interface. That is a risky default for a production-ish compose file, especially since the other app services already bind to localhost by default.Safer default
- ports: - - "5432:5432" + ports: + - "${HOST_POSTGRES_PORT:-127.0.0.1:5432:5432}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docker-compose.yml` around lines 65 - 66, The docker-compose service currently exposes Postgres on all interfaces via the ports mapping "5432:5432"; change the ports mapping for the Postgres service (look for the "ports:" entry and the "5432:5432" string) to bind only to localhost (e.g. use "127.0.0.1:5432:5432") or remove the host-side mapping entirely and rely on the internal Docker network so the database is not published externally by default.server/src/api/sites/addSite.ts-63-85 (1)
63-85:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject unknown
typevalues instead of coercing them to web.
FastifyRequestonly gives you TypeScript types here. At runtime, any unexpected string falls through to"web", which silently creates the wrong site type and bypasses the mobile-only guardrails in this handler.💡 Suggested fix
- const siteType = type === "mobile" ? "mobile" : "web"; + if (type != null && type !== "web" && type !== "mobile") { + return reply.status(400).send({ + error: "Invalid site type. Expected 'web' or 'mobile'", + }); + } + + const siteType = type === "mobile" ? "mobile" : "web";As per coding guidelines, "Validate untrusted request bodies and query params with Zod before using them in route handlers."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/api/sites/addSite.ts` around lines 63 - 85, The handler currently coerces any unknown type value into "web" via const siteType = type === "mobile" ? "mobile" : "web", which can create incorrect sites and skip mobile guards; change the logic to explicitly accept only "web" or "mobile" (reject any other string with a 400) or validate the request body with Zod before using it; specifically update the code around the siteType/type usage (and the early validation block that follows) to return a 400 error for unknown type values and/or replace the manual check with a Zod schema that enforces type: "web" | "mobile" so downstream checks (domainRegex/appIdentifierRegex and mobile-only feature guards) cannot be bypassed.react-native/index.js-383-392 (1)
383-392:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDeduplicate navigation events by path, not only route name.
This skips valid pageviews when one screen component is reused for multiple URLs or param sets, e.g.
/product/1→/product/2on the same route name.💡 Suggested fix
- let previousRouteName = null; + let previousRouteKey = null; @@ - const routeName = getRouteName(route); - if (!routeName || routeName === previousRouteName) return; - - previousRouteName = routeName; + const routeName = getRouteName(route); + const path = asPathname(getPath(route)); + const routeKey = `${routeName}:${path}`; + if (!routeName || routeKey === previousRouteKey) return; + + previousRouteKey = routeKey; await client.screen(routeName, includeRouteParams && route?.params ? { routeParams: route.params } : undefined, { - pathname: asPathname(getPath(route)), + pathname: path, screen: routeName, });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@react-native/index.js` around lines 383 - 392, trackCurrentRoute currently deduplicates by route name only (previousRouteName) which skips legitimate navigations between different paths using the same component; compute the current pathname (using asPathname(getPath(route)) or including route.params if needed) into a variable (e.g., currentPath), compare against a new previousPath variable instead of previousRouteName, return early if currentPath is unchanged, and then set previousPath = currentPath before calling client.screen (keep routeName for the screen label but use pathname/currentPath for deduplication).client/src/api/analytics/hooks/useGetMetric.ts-97-110 (1)
97-110:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove filter-only inputs from the lite query key.
combinedFiltersis correctly dropped from the request whenliteis true, butfilters,additionalFilters, andcustomFiltersstill participate in the key. That makes identical lite responses refetch and split cache entries whenever UI filters change.💡 Suggested fix
- return useQuery({ - queryKey: [parameter, customTime, time, site, filters, limit, page, additionalFilters, customFilters, timezone, lite], + return useQuery({ + queryKey: [parameter, customTime, time, site, combinedFilters, limit, page, timezone, lite], queryFn: async () => {As per coding guidelines, "Keep React Query keys as descriptive arrays and include every input that affects the response."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/src/api/analytics/hooks/useGetMetric.ts` around lines 97 - 110, The queryKey for useQuery still includes filter-only inputs even when lite is true; update the key to match the actual request inputs by excluding filters, additionalFilters, and customFilters when lite is true (since combinedFilters is already undefined for lite and used in buildApiParams). Concretely, adjust the queryKey construction in the useGetMetric hook (the useQuery call) to conditionally include the filter entries (filters, additionalFilters, customFilters) only when lite is false (or derive a keyFilters variable from combinedFilters and include that), so identical lite queries don’t produce separate cache entries.client/src/api/analytics/hooks/useGetOverviewLite.ts-11-20 (1)
11-20:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPass
timezoneintobuildApiParams()in both lite hooks.Both hooks vary their query key by timezone, but the request params do not. That makes date-based lite queries use the wrong backend window while still fragmenting the cache by timezone.
Suggested fix
export function useGetOverviewLite(site?: number | string) { const { time, timezone } = useStore(); - const params = buildApiParams(time); + const params = buildApiParams(time, timezone); @@ export function useGetOverviewBucketedLite({ @@ }) { const { time, timezone } = useStore(); - const params = buildApiParams(time); + const params = buildApiParams(time, timezone);As per coding guidelines, "Use 'buildApiParams(time, timezone, filters?)' utility to convert time store state into API-ready parameters"
Also applies to: 23-38
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/src/api/analytics/hooks/useGetOverviewLite.ts` around lines 11 - 20, The queryKey includes timezone but buildApiParams is called without it, causing requests to use the wrong date window while still fragmenting the cache; update useGetOverviewLite to call buildApiParams(time, timezone) (and do the same in the sibling lite hook referenced at lines 23-38) so the API params include timezone, leaving fetchOverviewLite(site!, params) and the queryKey unchanged.client/src/app/[site]/main/components/MainSection/OverviewLite.tsx-57-71 (1)
57-71:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHonor
pastMinutesEndwhen trimming sparkline points.This filter always uses
nowas the upper bound. For ranges like “120 to 60 minutes ago”, the chart can still show the trailing bucket from the last hour even though it is outside the selected window.Suggested fix
?.filter(d => { if (time.mode === "past-minutes") { const timestamp = new Date(d.time); const now = new Date(); const startTime = new Date(now.getTime() - time.pastMinutesStart * 60 * 1000); - return timestamp >= startTime && timestamp <= now; + const endTime = new Date(now.getTime() - time.pastMinutesEnd * 60 * 1000); + return timestamp >= startTime && timestamp <= endTime; } return true; })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/src/app/`[site]/main/components/MainSection/OverviewLite.tsx around lines 57 - 71, The sparklinesData filtering always uses now as the upper bound, ignoring the selected pastMinutesEnd, so update the filter in the sparklinesData computation (where time.mode, time.pastMinutesStart and getBucketValue are used) to compute both startTime and endTime based on now minus pastMinutesStart and now minus pastMinutesEnd (or use now if pastMinutesEnd is zero/null) and compare timestamp >= startTime && timestamp <= endTime; ensure you still map each entry to { value: getBucketValue ? getBucketValue(d) : d[id], time: d.time } and preserve existing behavior when time.mode !== "past-minutes".server/src/services/tracker/identifyService.ts-82-88 (1)
82-88:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not derive identity from client-supplied IP or user agent.
Preferring
ip_address/user_agentfrom the request body makes the anonymous fingerprint spoofable. A caller can forge another visitor's fingerprint, create an alias for it, and backfill that user's anonymous history onto an arbitraryuser_id.Suggested fix
const anonymousId = anonymous_id ? await userIdService.generateUserIdFromClientId(anonymous_id, siteId) : await userIdService.generateUserId( - ip_address || getIpAddress(request), - user_agent || request.headers["user-agent"] || "", + getIpAddress(request), + request.headers["user-agent"] || "", siteId );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/services/tracker/identifyService.ts` around lines 82 - 88, The current anonymousId construction uses client-supplied ip_address/user_agent from the request body which allows spoofing; update the logic in identifyService.ts where anonymous_id, generateUserIdFromClientId, generateUserId, ip_address, user_agent, getIpAddress, and request are referenced so that when anonymous_id is not provided you always call userIdService.generateUserId with server-derived values only (use getIpAddress(request) and request.headers["user-agent"] or ""), and ignore any ip_address or user_agent fields from the request body; keep generateUserIdFromClientId usage unchanged for the anonymous_id path.server/src/api/analytics/lite/utils.ts-55-64 (1)
55-64:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlign past-minute filtering to the MV's hour buckets.
This branch filters an hour-truncated column with minute-resolution timestamps. For recent windows like “last 30 minutes”, the current hour bucket gets excluded entirely, so lite charts/metrics can come back empty even though the MV has data.
Suggested fix
if (sanitized.pastMinutesRange) { const { start, end } = sanitized.pastMinutesRange; const now = new Date(); const startTimestamp = new Date(now.getTime() - start * 60 * 1000); const endTimestamp = new Date(now.getTime() - end * 60 * 1000); const startIso = startTimestamp.toISOString().slice(0, 19).replace("T", " "); const endIso = endTimestamp.toISOString().slice(0, 19).replace("T", " "); - return `AND ${column} > toDateTime(${SqlString.escape(startIso)}) AND ${column} <= toDateTime(${SqlString.escape(endIso)})`; + return `AND ${column} >= toStartOfHour(toDateTime(${SqlString.escape(startIso)})) + AND ${column} < toStartOfHour(toDateTime(${SqlString.escape(endIso)})) + INTERVAL 1 HOUR`; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/api/analytics/lite/utils.ts` around lines 55 - 64, The pastMinutesRange branch builds minute-resolution timestamps but the MV uses hour-truncated buckets, so floor both computed timestamps to their hour boundaries before creating the SQL predicate: compute startTimestamp and endTimestamp from sanitized.pastMinutesRange as before, then for each do timestamp.setMinutes(0,0,0) (or otherwise zero minutes/seconds) to get hour-truncated startHour and endHour, format those to ISO strings and use SqlString.escape(toHourIso) in the returned predicate (e.g. AND ${column} >= toDateTime(escapedStartHour) AND ${column} <= toDateTime(escapedEndHour)) so the filter aligns with the MV hour buckets.client/Dockerfile-33-40 (1)
33-40:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDeclare
ARG NEXT_PUBLIC_DEPLOYMENTin the builder stage before using it.
client/DockerfilesetsENV NEXT_PUBLIC_DEPLOYMENT=${NEXT_PUBLIC_DEPLOYMENT}in the builder stage, butARG NEXT_PUBLIC_DEPLOYMENTis not declared earlier in that stage—so the value expands to empty during the image build.Suggested fix
ARG NEXT_PUBLIC_BACKEND_URL ARG NEXT_PUBLIC_DISABLE_SIGNUP ARG NEXT_PUBLIC_CLOUD ARG NEXT_PUBLIC_TURNSTILE_SITE_KEY ARG NEXT_PUBLIC_STRIPE_PUBLISHABLE_KEY +ARG NEXT_PUBLIC_DEPLOYMENT ARG NEXT_PUBLIC_LITE_DASHBOARD ENV NEXT_PUBLIC_BACKEND_URL=${NEXT_PUBLIC_BACKEND_URL} ENV NEXT_PUBLIC_DISABLE_SIGNUP=${NEXT_PUBLIC_DISABLE_SIGNUP} ENV NEXT_PUBLIC_CLOUD=${NEXT_PUBLIC_CLOUD} ENV NEXT_PUBLIC_TURNSTILE_SITE_KEY=${NEXT_PUBLIC_TURNSTILE_SITE_KEY} ENV NEXT_PUBLIC_STRIPE_PUBLISHABLE_KEY=${NEXT_PUBLIC_STRIPE_PUBLISHABLE_KEY} ENV NEXT_PUBLIC_DEPLOYMENT=${NEXT_PUBLIC_DEPLOYMENT} ENV NEXT_PUBLIC_LITE_DASHBOARD=${NEXT_PUBLIC_LITE_DASHBOARD}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/Dockerfile` around lines 33 - 40, The builder stage in client/Dockerfile uses ENV NEXT_PUBLIC_DEPLOYMENT=${NEXT_PUBLIC_DEPLOYMENT} but never declares ARG NEXT_PUBLIC_DEPLOYMENT there, so the value expands empty; add an ARG NEXT_PUBLIC_DEPLOYMENT declaration in the builder stage (next to ARG NEXT_PUBLIC_LITE_DASHBOARD or other ARGs) before the ENV NEXT_PUBLIC_DEPLOYMENT line so the build-time variable is available when setting the environment variable.client/messages/pt.json-836-850 (1)
836-850:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon’t ship blank Portuguese strings for the new settings UI.
These new locale entries are still empty, so parts of the site settings/embed/tracking flows will show blank text in Portuguese. Please fill them in or intentionally fall back to English until the locale is ready.
As per coding guidelines, "Keep locale keys consistent across
messages/*.json."Also applies to: 1265-1282, 1383-1430
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/messages/pt.json` around lines 836 - 850, Several Portuguese locale entries are empty (e.g., keys "KI57sV", "AZEGID", "JkLHGw", "CXI0EW", "VQxsRK", "W35dz6" and other empty keys in the ranges noted) causing blank UI text; populate these keys with proper Portuguese translations or, if translation not ready, copy the English strings as intentional fallbacks to keep keys consistent across messages/*.json and prevent blank UI in the site settings/embed/tracking flows.client/messages/es.json-836-850 (1)
836-850:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon’t ship blank Spanish strings for the new settings UI.
These newly added Spanish entries are empty, so the site settings/embed/tracking flows will show missing text instead of localized copy. Please translate them or fall back to English rather than shipping blank strings.
As per coding guidelines, "Keep locale keys consistent across
messages/*.json."Also applies to: 1265-1282, 1383-1430
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/messages/es.json` around lines 836 - 850, Several message keys in the Spanish locale (e.g., "KI57sV", "AZEGID", "JkLHGw", "CXI0EW", "VQxsRK", "W35dz6") were left as empty strings and will render missing text in the settings/embed/tracking UI; for each of those keys in the messages JSON add proper Spanish translations or, if you cannot translate now, replace the empty value with the English source string as a fallback so the UI shows text, and ensure you keep the same locale keys consistent across other messages/*.json files as noted (also address the same empty keys at the other ranges mentioned).client/messages/cs.json-836-850 (1)
836-850:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon’t ship blank Czech strings for the new settings UI.
Several newly added entries in these sections are still
"", so labels and helper text in site settings/embed/tracking will render blank for Czech users. Please either provide translations here or fall back to the English copy until this locale is complete.As per coding guidelines, "Keep locale keys consistent across
messages/*.json."Also applies to: 1265-1282, 1383-1430
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/messages/cs.json` around lines 836 - 850, Several Czech localization entries for the new settings UI are empty and will render blank; fill in translations (or temporarily copy the English strings) for the empty keys such as "KI57sV", "AZEGID", "JkLHGw", "CXI0EW", "VQxsRK", and "W35dz6" (and the similar blank entries in the other ranges you flagged) so labels/helper text for site settings/embed/tracking are visible; keep the locale keys consistent with other messages/*.json files while updating the same symbolic keys across locales.server/src/api/analytics/lite/getOverviewBucketedLite.ts-117-151 (1)
117-151:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate the querystring before building the SQL.
This handler trusts
bucket,time_zone, and the date-range fields at runtime, but theFastifyRequesttype only helps TypeScript. A malformed query will currently flow intoliteBucket(...)/getLiteTimeStatement(...)and fail as a 500 instead of a 400.As per coding guidelines, "Validate untrusted request bodies and query params with Zod before using them in route handlers."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/api/analytics/lite/getOverviewBucketedLite.ts` around lines 117 - 151, The handler getOverviewBucketedLite is using unvalidated req.query values (bucket, time_zone, start_date/end_date/past_minutes_*) and passing them into liteBucket, getLiteTimeStatement, TimeBucketToFn and SqlString.escape which can cause 500s; add a Zod schema for the expected query params (bucket: TimeBucket enum, optional time_zone string, optional start_date/end_date/past_minutes_start/past_minutes_end) and validate req.query at the start of getOverviewBucketedLite, returning a 400 on validation failure, then use the parsed/typed values for liteBucket(...), getLiteTimeStatement(...), getLiteFillClause(...) and TimeBucketToFn lookups so only validated inputs reach those functions.client/messages/pl.json-836-850 (1)
836-850:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon’t ship blank Polish strings for the new settings UI.
A large chunk of the new locale entries is still
"", which means the Polish version of site settings/embed/tracking will render with missing copy. Please complete these translations or use a fallback instead of blank values.As per coding guidelines, "Keep locale keys consistent across
messages/*.json."Also applies to: 1265-1282, 1383-1430
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/messages/pl.json` around lines 836 - 850, Several locale entries in the Polish messages file are empty (e.g., keys KI57sV, AZEGID, JkLHGw, CXI0EW, VQxsRK, W35dz6 and the other ranges noted) causing missing UI copy for settings/embed/tracking; populate each empty string with an appropriate Polish translation or, if untranslated, copy the canonical English fallback value from the default messages file so the UI displays correctly. Ensure you update all empty keys in the other indicated ranges (around 1265-1282 and 1383-1430) and keep the locale keys identical to other messages/*.json files so the message lookup remains consistent across locales.client/src/components/SiteSettings/ScriptBuilder.tsx-79-112 (1)
79-112:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid using
globalThis.location.originduring render inScriptBuilder
client/src/components/SiteSettings/ScriptBuilder.tsxbuildstrackingScriptandreactNativeSnippetusingglobalThis.location.origindirectly in the component render path. Even with"use client", Next.js renders client components on the server for the initial HTML, wherelocationmay be unavailable—this can throw before hydration and break the settings page. Computeoriginonly on the client (e.g.,useEffect+ mounted gate) or pass it in from server/config, then render/build the snippets onceoriginis defined.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/src/components/SiteSettings/ScriptBuilder.tsx` around lines 79 - 112, The component ScriptBuilder currently uses globalThis.location.origin directly when constructing trackingScript and reactNativeSnippet which can run during server render; change ScriptBuilder to obtain origin only on the client (e.g. add a mounted gate using useState/useEffect or accept an origin prop from server) and defer building trackingScript and reactNativeSnippet until origin is defined; ensure you reference the same identifiers (trackingScript, reactNativeSnippet, siteId, appIdentifier) and guard any JSX that uses these strings so nothing accesses globalThis.location during SSR.client/messages/ko.json-1265-1283 (1)
1265-1283:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftBlank translations in active settings/embed flows will surface empty UI text
Several changed keys in this block are now
""(for example around Line 1271, Line 1276, Line 1282, Line 1304, Line 1306, Line 1328, Line 1420, Line 1422, Line 1430). These are user-visible labels/help text in dashboard embed and tracking settings, so the UI will show missing text in Korean.Also applies to: 1304-1307, 1328-1330, 1383-1385, 1420-1431
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/messages/ko.json` around lines 1265 - 1283, Some localization keys in client/messages/ko.json have been left as empty strings (e.g., "+CwN9C", "dmW5bt", "PkGCG1", "UcHF+A", "6NIu/x", "QRvU5C", "Ygzo5N", "KOXkeN", "PRAz+o", "WNF1J6", "oJEVob", "0uO8I3" and others in the embed/tracking block) which will render blank UI text; open the ko.json block around the dashboard embed/tracking keys and replace each "" with the correct Korean translation or a fallback string matching the English source so user-visible labels/help text (the dashboard embed and tracking settings) are not empty; ensure every affected key mentioned in the diff is populated and verify similar empty keys at the other ranges called out (1304-1307, 1328-1330, 1383-1385, 1420-1431).client/messages/de.json-1265-1283 (1)
1265-1283:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftGerman locale has newly blank strings in embed and tracking settings
A number of changed German messages in these ranges are now empty strings (not placeholders in dead code), which will surface as missing text in the UI. Please restore translations or fallback-safe values before merge.
Also applies to: 1304-1307, 1328-1330, 1383-1385, 1420-1431
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/messages/de.json` around lines 1265 - 1283, Several German locale entries were left as empty strings causing missing UI text; for each empty key shown (e.g. "+CwN9C", "dmW5bt", "PkGCG1", "UcHF+A", "6NIu/x", "QRvU5C", "Ygzo5N", "KOXkeN", "PRAz+o", "WNF1J6", "0uO8I3", "CHzo7t", etc.) and the other empty items in the mentioned ranges (1304-1307, 1328-1330, 1383-1385, 1420-1431) restore proper German translations or safe fallbacks (e.g. the English string) so the UI won’t show blanks; update the values in the de.json entries for those keys and verify that keys like "mBCW/X" ("Dashboard-Einbettung"), "FsIP28" ("Seitenleiste ausblenden"), "dZAp5K", "JfG49w", "TJo5E6", and "Pe0ogR" remain correct while populating the empty strings.client/messages/it.json-1265-1283 (1)
1265-1283:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftItalian translations were cleared in active UI flows
Changed entries across embed/domain/tracking settings are now
""(for example Line 1271, Line 1276, Line 1282, Line 1304, Line 1306, Line 1328, Line 1420, Line 1422, Line 1430). These should not ship empty because they drive visible labels and helper copy.Also applies to: 1304-1307, 1328-1330, 1383-1385, 1420-1431
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/messages/it.json` around lines 1265 - 1283, Several Italian translation keys used in embed/domain/tracking UI were accidentally cleared (e.g., "mBCW/X","FsIP28","dZAp5K","CHzo7t" and other empty keys like "+CwN9C","dmW5bt","PkGCG1"), causing visible labels/helper copy to be empty; restore accurate Italian strings for each emptied key in the same JSON structure (fill the empty values with the appropriate translations for embed/dashboard labels, toggles, helper text and "Embed code"/"Preview"/"Open" actions) and verify the related groups around the embed/domain/tracking settings (the blocks containing those keys) so no UI-facing label remains "" before merging.client/messages/fr.json-1265-1283 (1)
1265-1283:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftFrench locale regresses to blank strings in critical configuration screens
A large set of changed French messages is now empty (
"") in embed/domain/tracking sections (e.g., Line 1271, Line 1276, Line 1282, Line 1304, Line 1306, Line 1328, Line 1420, Line 1422, Line 1430). This will render blank text in active UI paths.Also applies to: 1304-1307, 1328-1330, 1383-1385, 1420-1431
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/messages/fr.json` around lines 1265 - 1283, Several French locale entries in client/messages/fr.json have been accidentally cleared to empty strings (e.g., keys "+CwN9C", "dmW5bt", "PkGCG1", "UcHF+A", "6NIu/x", "QRvU5C", "Ygzo5N", "KOXkeN", "PRAz+o", "oJEVob", "0uO8I3", etc.) causing blank UI labels in embed/domain/tracking and related screens; restore the original French translations for those keys (or provide appropriate new translations) so entries like those around "mBCW/X":"Intégration du tableau de bord", "Pe0ogR":"Thème", "FsIP28":"Masquer la barre latérale", "dZAp5K":"Seule la page principale du tableau de bord peut être consultée depuis cette intégration.", "JfG49w":"Ouverte", "TJo5E6":"Aperçu", "CHzo7t":"Code d'intégration" remain consistent and ensure no empty string values remain in the embed/domain/tracking sections before merging.
🧹 Nitpick comments (1)
client/src/app/[site]/main/components/sections/CountriesLite.tsx (1)
24-25: ⚡ Quick winConsider toggling the icon based on expanded state.
The button always shows the
Expandicon, even when the section is expanded. For better UX, consider using a conditional icon that switches betweenExpandandShrink(or similar) based on theexpandedstate.♻️ Proposed enhancement
+import { Expand, Shrink } from "lucide-react"; -import { Expand } from "lucide-react"; ... <Button size="smIcon" onClick={() => setExpanded(!expanded)}> - <Expand className="w-4 h-4" /> + {expanded ? <Shrink className="w-4 h-4" /> : <Expand className="w-4 h-4" />} </Button>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/src/app/`[site]/main/components/sections/CountriesLite.tsx around lines 24 - 25, The toggle button currently always renders the Expand icon; update the onClick render to choose the icon based on the expanded state (e.g., render Expand when !expanded and Shrink (or Collapse) when expanded) by changing the JSX inside the Button in CountriesLite.tsx to use a conditional like expanded ? <Shrink /> : <Expand /> and keep the existing onClick={() => setExpanded(!expanded)}; also add or adjust the import for the Shrink/Collapse icon component if it isn't already imported so the conditional icon renders correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d8979bf7-f761-4438-90b6-5c76a9f68e9d
📒 Files selected for processing (91)
client/Dockerfileclient/messages/cs.jsonclient/messages/de.jsonclient/messages/en.jsonclient/messages/es.jsonclient/messages/fr.jsonclient/messages/it.jsonclient/messages/ja.jsonclient/messages/ko.jsonclient/messages/pl.jsonclient/messages/pt.jsonclient/messages/uk.jsonclient/messages/zh.jsonclient/next.config.tsclient/src/api/admin/endpoints/adminSites.tsclient/src/api/admin/endpoints/sites.tsclient/src/api/analytics/endpoints/index.tsclient/src/api/analytics/endpoints/overview.tsclient/src/api/analytics/hooks/useGetMetric.tsclient/src/api/analytics/hooks/useGetOverviewLite.tsclient/src/app/[site]/components/Sidebar/Sidebar.tsxclient/src/app/[site]/components/Sidebar/SiteSelector.tsxclient/src/app/[site]/components/shared/StandardSection/StandardSection.tsxclient/src/app/[site]/components/shared/StandardSection/StandardSectionDialog.tsxclient/src/app/[site]/main/components/MainSection/MainSectionLite.tsxclient/src/app/[site]/main/components/MainSection/OverviewLite.tsxclient/src/app/[site]/main/components/sections/CountriesLite.tsxclient/src/app/[site]/main/components/sections/DevicesLite.tsxclient/src/app/[site]/main/components/sections/PagesLite.tsxclient/src/app/[site]/main/page.tsxclient/src/app/components/AddSite.tsxclient/src/app/rollup/components/MainSection/MainSection.tsxclient/src/app/rollup/components/sections/CountriesLite.tsxclient/src/app/rollup/components/sections/PagesLite.tsxclient/src/app/rollup/components/sections/RollupSection.tsxclient/src/app/rollup/components/sections/RollupSectionDialog.tsxclient/src/app/rollup/hooks/useRollupBucketed.tsclient/src/app/rollup/hooks/useRollupMetric.tsclient/src/app/rollup/page.tsxclient/src/components/AppSidebar.tsxclient/src/components/SiteSettings/GeneralTab.tsxclient/src/components/SiteSettings/ScriptBuilder.tsxclient/src/components/SiteSettings/SiteSettings.tsxclient/src/components/SiteSettings/TrackingTab.tsxclient/src/lib/configs.tsclient/src/lib/const.tsdocker-compose.ymldocs/content/docs/sdks/meta.jsondocs/content/docs/sdks/react-native.mdxdocs/messages/de.jsondocs/messages/en.jsondocs/messages/es.jsondocs/messages/fr.jsondocs/messages/it.jsondocs/messages/ja.jsondocs/messages/ko.jsondocs/messages/pl.jsondocs/messages/pt.jsondocs/messages/zh.jsonreact-native/README.mdreact-native/index.d.tsreact-native/index.jsreact-native/package.jsonserver/drizzle/0007_site_type.sqlserver/drizzle/meta/0006_snapshot.jsonserver/drizzle/meta/0007_snapshot.jsonserver/drizzle/meta/_journal.jsonserver/package.jsonserver/src/api/admin/getAdminSites.tsserver/src/api/analytics/index.tsserver/src/api/analytics/lite/getMetricLite.tsserver/src/api/analytics/lite/getOverviewBucketedLite.tsserver/src/api/analytics/lite/getOverviewLite.tsserver/src/api/analytics/lite/utils.tsserver/src/api/getConfig.tsserver/src/api/sites/addSite.tsserver/src/api/sites/getSite.tsserver/src/api/sites/getSitesFromOrg.tsserver/src/api/sites/getTrackingConfig.tsserver/src/api/sites/updateSiteConfig.tsserver/src/cluster.tsserver/src/db/clickhouse/clickhouse.tsserver/src/db/postgres/schema.tsserver/src/index.tsserver/src/lib/const.tsserver/src/lib/siteConfig.tsserver/src/scripts/backfillLiteDashboardMVs.tsserver/src/services/tracker/identifyService.tsserver/src/services/tracker/trackEvent.tsserver/src/services/tracker/utils.tsserver/src/services/userId/userIdService.ts
- Introduced a new API endpoint for creating users within an organization, enhancing user management capabilities. - Updated the frontend to utilize the new create user functionality, replacing the previous add user method. - Added necessary types and hooks to support the new feature, ensuring a seamless integration into the existing workflow.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server/src/api/analytics/lite/utils.ts (1)
89-124:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReuse
validateTimeStatementParamsingetLiteFillClauseto prevent runtimeRangeErrorand malformedWITH FILLSQL
getOverviewBucketedLitecallsgetLiteFillClause(req.query, bucket)directly (thepublicSitepreHandler only resolves/authorizessiteId), whilegetLiteFillClauseitself does not sanitize/parse. As a result:
- Invalid
past_minutes_start/past_minutes_end(e.g., non-numeric strings) makeNumber(...)produceNaN, sonew Date(NaN).toISOString()can throwRangeError: Invalid time value→ 500 (server/src/api/analytics/lite/utils.ts).- Invalid
start_date/end_date/time_zonecan lead to a ClickHouse-rejectedWITH FILLfragment instead of being safely rejected/sanitized (server/src/api/analytics/lite/utils.ts, call siteserver/src/api/analytics/lite/getOverviewBucketedLite.ts).Refactor
getLiteFillClauseto usevalidateTimeStatementParams(parity withgetLiteTimeStatement) and build the fill clause only from the sanitized result.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/api/analytics/lite/utils.ts` around lines 89 - 124, Call validateTimeStatementParams at the start of getLiteFillClause and use its sanitized return values instead of reading raw params directly: replace direct access to params.{start_date,end_date,time_zone,past_minutes_start,past_minutes_end} and any Number()/new Date()/toISOString() calls with the validated/sanitized fields returned by validateTimeStatementParams so malformed inputs won't produce NaN/RangeError or invalid WITH FILL fragments; keep the same SQL construction logic and references to TimeBucketToFn and bucketIntervalMap but build the FROM/TO/STEP clauses from the validated values (mirroring how getLiteTimeStatement is implemented) so getOverviewBucketedLite can safely call getLiteFillClause.client/src/api/analytics/hooks/useGetMetric.ts (1)
107-108:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix React Query cache keys to include
useFilters.In
usePaginatedMetric(around lines 107–108) anduseInfiniteMetric(around 161–174),useFilterschanges request params viacombinedFilters, but neitherqueryKeyincludesuseFilters, so filtered/unfiltered calls can reuse the wrong cache. AdduseFiltersto both query keys.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/src/api/analytics/hooks/useGetMetric.ts` around lines 107 - 108, Both usePaginatedMetric and useInfiniteMetric build request params with combinedFilters from useFilters but the queryKey arrays (in the useQuery/useInfiniteQuery calls) don't include useFilters, causing cached results to be reused incorrectly; update the queryKey entries in usePaginatedMetric (the useQuery call around the returned queryKey currently listing parameter, customTime, time, site, filters, limit, page, additionalFilters, customFilters, timezone, lite) and in useInfiniteMetric (the useInfiniteQuery call around lines 161–174) to include useFilters so the cache differentiates filtered vs unfiltered requests.
🧹 Nitpick comments (1)
server/src/api/user/createUserInOrganization.ts (1)
63-76: ⚡ Quick winPrefer Zod for body validation.
The body is validated manually. Validating
request.bodywith a Zod schema (email/name/password/role) would centralize the rules and guard against missing/malformed bodies before destructuring at Line 46. As per coding guidelines, "Validate untrusted request bodies and query params with Zod before using them in route handlers."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/api/user/createUserInOrganization.ts` around lines 63 - 76, Replace the manual validation in createUserInOrganization with a Zod schema: define a schema (e.g., CreateUserBodySchema) that requires email (email()), name (optional/string), password (min length 8), and role (enum of "admin","member","owner"), then validate request.body with schema.parse or schema.safeParse at the top of the handler before you read rawEmail/password/role; on validation failure return reply.status(400) with the Zod error details so subsequent code (where rawEmail, password, role are used) can safely assume the types/constraints.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/src/api/analytics/lite/getMetricLite.ts`:
- Line 39: The current limit calculation (const limit =
Math.min(Number(req.query.limit) || 100, 500)) allows negative values and
non-integers; change it to parse and clamp to a positive integer:
parseInt(req.query.limit, 10) (fall back to 100 if NaN), then apply Math.max(1,
Math.min(parsed, 500)) so limit is an integer between 1 and 500. Update the
const limit in getMetricLite (the line using Number(req.query.limit)) to use
this parsing and clamping approach.
In `@server/src/api/user/createUserInOrganization.ts`:
- Around line 84-114: The current flow creates a user via
ctx.internalAdapter.createUser and ctx.internalAdapter.linkAccount then inserts
the org member via db.insert(member); if the member insert fails the created
user/account are orphaned—wrap the member insert in a try/catch and on failure
perform compensating cleanup by calling the appropriate adapter methods to
remove the created credential and user (e.g., reverse
ctx.internalAdapter.linkAccount and ctx.internalAdapter.deleteUser or a provided
remove method for account/user) and then return a 500; alternatively, detect an
existing user with no membership earlier and attach the new member record to
that user instead of creating a new account (check in the duplicate-email branch
for a membership-less user and insert member for that existing user) so retries
do not leave unusable accounts.
- Around line 115-118: In the catch block of createUserInOrganization
(server/src/api/user/createUserInOrganization.ts) stop returning String(error)
to clients; instead log the full error server-side (e.g., console.error or your
app logger with the caught error object) and return a generic 500 response like
{ error: "Internal server error" } to the client; update the catch clause around
the createUserInOrganization logic to use the logger for detailed error output
and change reply.status(500).send(...) to a non-sensitive generic message.
- Around line 49-72: The handler createUserInOrganization currently allows
org-level admins to create a new owner; after the existing role validation, add
a guard that if role === "owner" then only permit it when the requester is a
system admin (isAdmin === true) or the requester's userMembership.role ===
"owner"; otherwise return an appropriate 403/401 response. Use the existing
isAdmin and userMembership variables (ensure userMembership is checked for
existence when isAdmin is false) to enforce this restriction.
---
Outside diff comments:
In `@client/src/api/analytics/hooks/useGetMetric.ts`:
- Around line 107-108: Both usePaginatedMetric and useInfiniteMetric build
request params with combinedFilters from useFilters but the queryKey arrays (in
the useQuery/useInfiniteQuery calls) don't include useFilters, causing cached
results to be reused incorrectly; update the queryKey entries in
usePaginatedMetric (the useQuery call around the returned queryKey currently
listing parameter, customTime, time, site, filters, limit, page,
additionalFilters, customFilters, timezone, lite) and in useInfiniteMetric (the
useInfiniteQuery call around lines 161–174) to include useFilters so the cache
differentiates filtered vs unfiltered requests.
In `@server/src/api/analytics/lite/utils.ts`:
- Around line 89-124: Call validateTimeStatementParams at the start of
getLiteFillClause and use its sanitized return values instead of reading raw
params directly: replace direct access to
params.{start_date,end_date,time_zone,past_minutes_start,past_minutes_end} and
any Number()/new Date()/toISOString() calls with the validated/sanitized fields
returned by validateTimeStatementParams so malformed inputs won't produce
NaN/RangeError or invalid WITH FILL fragments; keep the same SQL construction
logic and references to TimeBucketToFn and bucketIntervalMap but build the
FROM/TO/STEP clauses from the validated values (mirroring how
getLiteTimeStatement is implemented) so getOverviewBucketedLite can safely call
getLiteFillClause.
---
Nitpick comments:
In `@server/src/api/user/createUserInOrganization.ts`:
- Around line 63-76: Replace the manual validation in createUserInOrganization
with a Zod schema: define a schema (e.g., CreateUserBodySchema) that requires
email (email()), name (optional/string), password (min length 8), and role (enum
of "admin","member","owner"), then validate request.body with schema.parse or
schema.safeParse at the top of the handler before you read
rawEmail/password/role; on validation failure return reply.status(400) with the
Zod error details so subsequent code (where rawEmail, password, role are used)
can safely assume the types/constraints.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 430186f2-a3c2-45eb-8249-610c28827302
📒 Files selected for processing (13)
client/src/api/admin/endpoints/index.tsclient/src/api/admin/endpoints/organizations.tsclient/src/api/admin/hooks/useOrganizations.tsclient/src/api/analytics/hooks/useGetMetric.tsclient/src/api/analytics/hooks/useGetOverviewLite.tsclient/src/app/settings/organization/components/CreateUserDialog.tsxserver/src/api/analytics/lite/getMetricLite.tsserver/src/api/analytics/lite/getOverviewBucketedLite.tsserver/src/api/analytics/lite/getOverviewLite.tsserver/src/api/analytics/lite/utils.tsserver/src/api/user/createUserInOrganization.tsserver/src/api/user/index.tsserver/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- server/src/api/analytics/lite/getOverviewLite.ts
- client/src/api/analytics/hooks/useGetOverviewLite.ts
- server/src/api/analytics/lite/getOverviewBucketedLite.ts
|
|
||
| const site = Number(req.params.siteId); | ||
| const { parameter } = req.query; | ||
| const limit = Math.min(Number(req.query.limit) || 100, 500); |
Contributor
There was a problem hiding this comment.
Clamp limit to a positive integer.
Number(req.query.limit) || 100 lets negatives and fractions through (e.g. ?limit=-5 → LIMIT -5, ?limit=50.5 → LIMIT 50.5), which ClickHouse rejects and surfaces as a 500.
🔧 Proposed fix
- const limit = Math.min(Number(req.query.limit) || 100, 500);
+ const limit = Math.min(Math.max(Math.floor(Number(req.query.limit)) || 100, 1), 500);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/src/api/analytics/lite/getMetricLite.ts` at line 39, The current limit
calculation (const limit = Math.min(Number(req.query.limit) || 100, 500)) allows
negative values and non-integers; change it to parse and clamp to a positive
integer: parseInt(req.query.limit, 10) (fall back to 100 if NaN), then apply
Math.max(1, Math.min(parsed, 500)) so limit is an integer between 1 and 500.
Update the const limit in getMetricLite (the line using Number(req.query.limit))
to use this parsing and clamping approach.
Comment on lines
+49
to
+72
| // Authorization: system admin OR owner/admin of this organization | ||
| const isAdmin = await getIsUserAdmin(request); | ||
| if (!isAdmin) { | ||
| if (!userId) { | ||
| return reply.status(401).send({ error: "Unauthorized" }); | ||
| } | ||
| const userMembership = await db.query.member.findFirst({ | ||
| where: and(eq(member.userId, userId), eq(member.organizationId, organizationId)), | ||
| }); | ||
| if (!userMembership || (userMembership.role !== "admin" && userMembership.role !== "owner")) { | ||
| return reply.status(401).send({ error: "Unauthorized" }); | ||
| } | ||
| } | ||
|
|
||
| // Validate input | ||
| if (!rawEmail || !password || !role) { | ||
| return reply.status(400).send({ error: "Missing required fields: email, password and role" }); | ||
| } | ||
|
|
||
| const email = rawEmail.toLowerCase(); | ||
|
|
||
| if (role !== "admin" && role !== "member" && role !== "owner") { | ||
| return reply.status(400).send({ error: "Role must be either admin, member, or owner" }); | ||
| } |
Contributor
There was a problem hiding this comment.
Privilege escalation: an org admin can mint an owner.
The authorization at Line 58 admits any admin or owner member, but the role check at Line 70 permits creating an owner regardless of the requester's role. A non-system, org-level admin can therefore create a new owner account they control and escalate privileges. Restrict assigning the owner role to system admins and existing owners.
🔒 Proposed fix to gate the owner role
const isAdmin = await getIsUserAdmin(request);
+ let requesterRole: string | undefined;
if (!isAdmin) {
if (!userId) {
return reply.status(401).send({ error: "Unauthorized" });
}
const userMembership = await db.query.member.findFirst({
where: and(eq(member.userId, userId), eq(member.organizationId, organizationId)),
});
if (!userMembership || (userMembership.role !== "admin" && userMembership.role !== "owner")) {
return reply.status(401).send({ error: "Unauthorized" });
}
+ requesterRole = userMembership.role;
}…then, after the role validation:
if (role !== "admin" && role !== "member" && role !== "owner") {
return reply.status(400).send({ error: "Role must be either admin, member, or owner" });
}
+
+ // Only system admins or existing owners may grant the owner role.
+ if (role === "owner" && !isAdmin && requesterRole !== "owner") {
+ return reply.status(403).send({ error: "Only owners can assign the owner role" });
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/src/api/user/createUserInOrganization.ts` around lines 49 - 72, The
handler createUserInOrganization currently allows org-level admins to create a
new owner; after the existing role validation, add a guard that if role ===
"owner" then only permit it when the requester is a system admin (isAdmin ===
true) or the requester's userMembership.role === "owner"; otherwise return an
appropriate 403/401 response. Use the existing isAdmin and userMembership
variables (ensure userMembership is checked for existence when isAdmin is false)
to enforce this restriction.
Comment on lines
+84
to
+114
| // Create the user + credential account (mirrors better-auth's admin createUser) | ||
| const createdUser = await ctx.internalAdapter.createUser({ | ||
| email, | ||
| name: name || email, | ||
| emailVerified: false, | ||
| }); | ||
|
|
||
| if (!createdUser) { | ||
| return reply.status(500).send({ error: "Failed to create user" }); | ||
| } | ||
|
|
||
| const hashedPassword = await ctx.password.hash(password); | ||
| await ctx.internalAdapter.linkAccount({ | ||
| accountId: createdUser.id, | ||
| providerId: "credential", | ||
| password: hashedPassword, | ||
| userId: createdUser.id, | ||
| }); | ||
|
|
||
| // Add the new user to the organization | ||
| await db.insert(member).values([ | ||
| { | ||
| userId: createdUser.id, | ||
| organizationId: organizationId, | ||
| role: role, | ||
| id: generateId(), | ||
| createdAt: new Date().toISOString(), | ||
| }, | ||
| ]); | ||
|
|
||
| return reply.status(201).send({ message: "User created and added to organization successfully" }); |
Contributor
There was a problem hiding this comment.
Partial failure leaves an orphaned user that can never be retried.
User creation (createUser + linkAccount) and the member insert are not atomic and span two systems. If the db.insert(member) at Line 104 fails, the user/credential account already exists, so every retry now short-circuits at the duplicate check (Lines 80-82) with "A user with this email already exists" — yet the user was never added to the org. Add compensating cleanup (or reconcile an existing membership-less user) so failures don't leave an unusable account.
Want me to draft a version that rolls back the created user/account on member-insert failure (or links membership to a pre-existing membership-less user)?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/src/api/user/createUserInOrganization.ts` around lines 84 - 114, The
current flow creates a user via ctx.internalAdapter.createUser and
ctx.internalAdapter.linkAccount then inserts the org member via
db.insert(member); if the member insert fails the created user/account are
orphaned—wrap the member insert in a try/catch and on failure perform
compensating cleanup by calling the appropriate adapter methods to remove the
created credential and user (e.g., reverse ctx.internalAdapter.linkAccount and
ctx.internalAdapter.deleteUser or a provided remove method for account/user) and
then return a 500; alternatively, detect an existing user with no membership
earlier and attach the new member record to that user instead of creating a new
account (check in the duplicate-email branch for a membership-less user and
insert member for that existing user) so retries do not leave unusable accounts.
Comment on lines
+115
to
+118
| } catch (error: any) { | ||
| console.error(String(error)); | ||
| return reply.status(500).send({ error: String(error) }); | ||
| } |
Contributor
There was a problem hiding this comment.
Don't return raw error strings to the client.
String(error) can surface internal details (DB/driver messages, query fragments) to the caller. Log the full error server-side and return a generic message.
🛡️ Proposed fix
} catch (error: any) {
- console.error(String(error));
- return reply.status(500).send({ error: String(error) });
+ console.error("Error creating user in organization:", error);
+ return reply.status(500).send({ error: "Failed to create user" });
}📝 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
| } catch (error: any) { | |
| console.error(String(error)); | |
| return reply.status(500).send({ error: String(error) }); | |
| } | |
| } catch (error: any) { | |
| console.error("Error creating user in organization:", error); | |
| return reply.status(500).send({ error: "Failed to create user" }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/src/api/user/createUserInOrganization.ts` around lines 115 - 118, In
the catch block of createUserInOrganization
(server/src/api/user/createUserInOrganization.ts) stop returning String(error)
to clients; instead log the full error server-side (e.g., console.error or your
app logger with the caught error object) and return a generic 500 response like
{ error: "Internal server error" } to the client; update the catch clause around
the createUserInOrganization logic to use the logger for detailed error output
and change reply.status(500).send(...) to a non-sensitive generic message.
…o use StandardSectionTabs - Replaced the previous expandable section layout with StandardSectionTabs for a more streamlined user experience. - Updated the components to define tabs for countries, devices, and pages, enhancing organization and accessibility of information. - Removed unnecessary state management and UI elements, simplifying the component structure.
- Changed the default limit for metrics in getMetricLite from 100 to 250, enhancing the data retrieval process. - This adjustment aims to provide a more comprehensive dataset while still capping the maximum limit at 500.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
server/src/api/analytics/lite/getMetricLite.ts (2)
130-189:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRemove the committed benchmark block with the live bearer token.
This block still contains a real-looking API key and internal endpoint details. Even inside a comment, that's a credential exposure and should not ship. Move the recipe to internal docs and replace every secret with obvious placeholders before keeping any of it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/api/analytics/lite/getMetricLite.ts` around lines 130 - 189, Delete the committed benchmark block that contains the live bearer token and internal endpoints (the commented section containing BASE_URL, API_KEY, SITE_ID, START, END, TZ and the curl bench/warmup scripts); either remove it entirely from getMetricLite.ts or replace every sensitive value (API_KEY, BASE_URL, SITE_ID) with obvious placeholders like "<API_KEY_PLACEHOLDER>" and add a short comment pointing to an internal docs location for the recipe; ensure no real secrets remain in the file.
39-39:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClamp
limitto a positive integer before interpolating it into SQL.
Number(req.query.limit) || 250still accepts values like-5or50.5, which then flow intoLIMIT ${limit}and turn a bad request into a 500. Please parse and bound this to an integer in the1..500range first. As per coding guidelines, "Validate untrusted request bodies and query params with Zod before using them in route handlers."🔧 Proposed fix
- const limit = Math.min(Number(req.query.limit) || 250, 500); + const parsedLimit = Number.parseInt(String(req.query.limit ?? 250), 10); + const limit = Math.max(1, Math.min(Number.isNaN(parsedLimit) ? 250 : parsedLimit, 500));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/api/analytics/lite/getMetricLite.ts` at line 39, The current line computing limit allows negative or non-integer values into the SQL LIMIT; update the request validation so req.query.limit is parsed as an integer and clamped to the 1..500 range before interpolation. Specifically, convert req.query.limit to a numeric integer (e.g., parseInt/Math.floor), handle NaN by defaulting to 250, then bound it with Math.max(1, Math.min(parsedLimit, 500)) and use that variable (still named limit) in the SQL; additionally, add a Zod schema for the query (or validate early in the route handler) to ensure limit is an integer within 1..500 before executing getMetricLite logic.
🧹 Nitpick comments (3)
client/src/app/[site]/main/components/sections/PagesLite.tsx (1)
3-36: ⚡ Quick winUse
useTranslations()for the new user-facing labels.This new section also uses
useExtracted()for visible copy. Theclient/srcconvention isuseTranslations(), so it would be better to align this file now rather than introduce a second i18n pattern. As per coding guidelines, "Use next-intl's 'useTranslations()' hook for i18n; maintain translation files in messages/ directory (en.json, de.json, etc.)."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/src/app/`[site]/main/components/sections/PagesLite.tsx around lines 3 - 36, Replace useExtracted with next-intl's useTranslations in PagesLite: change the import and the hook call (remove useExtracted, import and call useTranslations in PagesLite), then update t("...") usage to match useTranslations semantics (same string keys) for labels in the tabs definition (label, section.title, getLabel fallback). Ensure imports at top reflect useTranslations and remove unused useExtracted; keep siteMetadata usage and StandardSectionTabs integration unchanged.client/src/app/[site]/main/components/sections/CountriesLite.tsx (1)
3-36: ⚡ Quick winUse
useTranslations()for the new user-facing labels.This component uses
useExtracted()for visible copy, but theclient/srci18n guideline standardizes onuseTranslations()for user-facing strings. Please switch this new section to the shared pattern before it spreads further. As per coding guidelines, "Use next-intl's 'useTranslations()' hook for i18n; maintain translation files in messages/ directory (en.json, de.json, etc.)."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/src/app/`[site]/main/components/sections/CountriesLite.tsx around lines 3 - 36, The component CountriesLite currently imports and calls useExtracted() for visible UI text; replace that with next-intl's useTranslations() hook to follow client/src i18n guidelines: import useTranslations, replace const t = useExtracted() with const t = useTranslations(/* optional namespace */) and keep all uses of t("...") unchanged, ensure translation keys exist in messages/*.json (e.g., en.json) and remove the useExtracted import; update any tests or type usages if they reference useExtracted in CountriesLite.client/src/app/[site]/main/components/sections/DevicesLite.tsx (1)
3-34: ⚡ Quick winUse
useTranslations()for the new user-facing labels.This file introduces another
useExtracted()call on visible UI text. Please keep the new lite sections on the standarduseTranslations()path used inclient/src. As per coding guidelines, "Use next-intl's 'useTranslations()' hook for i18n; maintain translation files in messages/ directory (en.json, de.json, etc.)."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/src/app/`[site]/main/components/sections/DevicesLite.tsx around lines 3 - 34, The component DevicesLite is using useExtracted() for visible UI text; replace it with next-intl's useTranslations() to follow the project's i18n convention: import useTranslations from "next-intl", change the t = useExtracted() to t = useTranslations(/* appropriate namespace or key prefix used across client/src */), and keep all existing uses of t (label, title, getLabel fallback) intact; ensure the translation keys referenced (e.g., "Devices" and "Other") exist in the messages/ JSON files so the StandardSectionTabs (tabs, getLabel, lite) renders with the correct localized strings.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@client/src/components/BucketSelection.tsx`:
- Around line 77-82: availableOptions filtering can yield an empty list in lite
mode because the LITE_DASHBOARD branch removes sub-hour options and the existing
durationMinutes check still blocks "hour"; update the logic in the
availableOptions calculation (the filter that uses LITE_DASHBOARD and
bucketDurationMinutes(option)) to provide a lite-mode fallback: when
LITE_DASHBOARD is true and the filtered result would be empty, include at least
"hour" (or the first valid coarser bucket from getOptions()) so the
Select/SelectItem list never renders empty; use the existing helpers
bucketDurationMinutes(option) and getOptions()/durationMinutes to pick the
fallback option and ensure the <Select> always has at least one SelectItem.
---
Duplicate comments:
In `@server/src/api/analytics/lite/getMetricLite.ts`:
- Around line 130-189: Delete the committed benchmark block that contains the
live bearer token and internal endpoints (the commented section containing
BASE_URL, API_KEY, SITE_ID, START, END, TZ and the curl bench/warmup scripts);
either remove it entirely from getMetricLite.ts or replace every sensitive value
(API_KEY, BASE_URL, SITE_ID) with obvious placeholders like
"<API_KEY_PLACEHOLDER>" and add a short comment pointing to an internal docs
location for the recipe; ensure no real secrets remain in the file.
- Line 39: The current line computing limit allows negative or non-integer
values into the SQL LIMIT; update the request validation so req.query.limit is
parsed as an integer and clamped to the 1..500 range before interpolation.
Specifically, convert req.query.limit to a numeric integer (e.g.,
parseInt/Math.floor), handle NaN by defaulting to 250, then bound it with
Math.max(1, Math.min(parsedLimit, 500)) and use that variable (still named
limit) in the SQL; additionally, add a Zod schema for the query (or validate
early in the route handler) to ensure limit is an integer within 1..500 before
executing getMetricLite logic.
---
Nitpick comments:
In `@client/src/app/`[site]/main/components/sections/CountriesLite.tsx:
- Around line 3-36: The component CountriesLite currently imports and calls
useExtracted() for visible UI text; replace that with next-intl's
useTranslations() hook to follow client/src i18n guidelines: import
useTranslations, replace const t = useExtracted() with const t =
useTranslations(/* optional namespace */) and keep all uses of t("...")
unchanged, ensure translation keys exist in messages/*.json (e.g., en.json) and
remove the useExtracted import; update any tests or type usages if they
reference useExtracted in CountriesLite.
In `@client/src/app/`[site]/main/components/sections/DevicesLite.tsx:
- Around line 3-34: The component DevicesLite is using useExtracted() for
visible UI text; replace it with next-intl's useTranslations() to follow the
project's i18n convention: import useTranslations from "next-intl", change the t
= useExtracted() to t = useTranslations(/* appropriate namespace or key prefix
used across client/src */), and keep all existing uses of t (label, title,
getLabel fallback) intact; ensure the translation keys referenced (e.g.,
"Devices" and "Other") exist in the messages/ JSON files so the
StandardSectionTabs (tabs, getLabel, lite) renders with the correct localized
strings.
In `@client/src/app/`[site]/main/components/sections/PagesLite.tsx:
- Around line 3-36: Replace useExtracted with next-intl's useTranslations in
PagesLite: change the import and the hook call (remove useExtracted, import and
call useTranslations in PagesLite), then update t("...") usage to match
useTranslations semantics (same string keys) for labels in the tabs definition
(label, section.title, getLabel fallback). Ensure imports at top reflect
useTranslations and remove unused useExtracted; keep siteMetadata usage and
StandardSectionTabs integration unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1d3ea7dc-397f-4939-9d49-6b2cf0a6677c
📒 Files selected for processing (6)
client/src/app/[site]/main/components/sections/CountriesLite.tsxclient/src/app/[site]/main/components/sections/DevicesLite.tsxclient/src/app/[site]/main/components/sections/PagesLite.tsxclient/src/components/BucketSelection.tsxclient/src/lib/store.tsserver/src/api/analytics/lite/getMetricLite.ts
Comment on lines
77
to
+82
| const availableOptions = options.filter( | ||
| option => durationMinutes === null || durationMinutes >= bucketDurationMinutes(option) * 2 | ||
| option => | ||
| // The lite dashboard is backed by hourly materialized views, so buckets | ||
| // finer than an hour have no underlying data. | ||
| (!LITE_DASHBOARD || bucketDurationMinutes(option) >= 60) && | ||
| (durationMinutes === null || durationMinutes >= bucketDurationMinutes(option) * 2) |
Contributor
There was a problem hiding this comment.
Avoid rendering an empty bucket selector in lite mode.
For past-minutes ranges of 120 minutes or less, getOptions() only passes ["minute"]. This new lite filter removes that option, and the existing durationMinutes >= bucketDurationMinutes(option) * 2 check still blocks "hour" for shorter windows, so the <Select> can end up with no SelectItems at all. Please add a lite fallback here instead of applying the normal sub-hour filter blindly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@client/src/components/BucketSelection.tsx` around lines 77 - 82,
availableOptions filtering can yield an empty list in lite mode because the
LITE_DASHBOARD branch removes sub-hour options and the existing durationMinutes
check still blocks "hour"; update the logic in the availableOptions calculation
(the filter that uses LITE_DASHBOARD and bucketDurationMinutes(option)) to
provide a lite-mode fallback: when LITE_DASHBOARD is true and the filtered
result would be empty, include at least "hour" (or the first valid coarser
bucket from getOptions()) so the Select/SelectItem list never renders empty; use
the existing helpers bucketDurationMinutes(option) and
getOptions()/durationMinutes to pick the fallback option and ensure the <Select>
always has at least one SelectItem.
…ination support - Updated the analytics API to support session filters in lite metric queries, allowing for more granular data retrieval based on session-related dimensions. - Implemented pagination in the getMetricLite function to improve data handling and user experience when fetching metrics. - Refactored SQL queries in getOverviewBucketedLite and getOverviewLite to accommodate session filters, ensuring efficient data processing and accurate results. - Enhanced utility functions for better filter management, streamlining the overall analytics workflow.
- Introduced a new LazySection for the Events component in MainPageContent, improving the overall layout and user experience. - Set a specific height for the Events section to ensure consistent display alongside other sections.
…r readability - Reformatted the LazySection components in MainPageContent to enhance code readability and maintainability. - Adjusted the height of the Events section for consistent display across different screen sizes.
- Updated useGetOverview and useGetOverviewBucketed hooks to include a new 'lite' option, allowing for the use of MV-backed lite endpoints. - Removed the deprecated useGetOverviewLite and useGetOverviewBucketedLite hooks to streamline the codebase. - Adjusted components to utilize the new lite functionality, ensuring consistent data retrieval across the application.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
server/src/api/analytics/lite/getMetricLite.ts (1)
36-38:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClamp
limitandpageto positive integers.
Number(...)lets negatives and fractions through.?limit=-5/?limit=50.5yieldsLIMIT -5/LIMIT 50.5, and a fractionalpage(e.g.?page=1.3) produces a fractionalOFFSET— both rejected by ClickHouse and surfaced as a 500. Thelimitissue echoes earlier feedback and remains unaddressed;page/offsethave the same gap.🔧 Proposed fix
- const limit = Math.min(Number(req.query.limit) || 250, 500); - const page = Math.max(Number(req.query.page) || 1, 1); + const limit = Math.min(Math.max(Math.floor(Number(req.query.limit)) || 250, 1), 500); + const page = Math.max(Math.floor(Number(req.query.page)) || 1, 1); const offsetStatement = page > 1 ? `OFFSET ${(page - 1) * limit}` : "";As per coding guidelines, "Validate untrusted request bodies and query params ... before using them in route handlers."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/api/analytics/lite/getMetricLite.ts` around lines 36 - 38, The limit and page query params (variables limit, page, and offsetStatement in getMetricLite.ts) must be coerced to positive integers and clamped before use: parse/convert the incoming req.query.limit and req.query.page to integers (e.g., via parseInt or Math.floor after Number()), default to 250 and 1 respectively, clamp limit to the range [1, 500] and ensure page is >=1, and then compute offset using these integer values so OFFSET and LIMIT are always whole positive numbers; update the offsetStatement construction to use the sanitized page and limit variables.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/src/api/analytics/lite/utils.ts`:
- Around line 50-56: wrapLiteLikeValue currently inserts % wildcards around raw
input without escaping LIKE metacharacters, causing overmatching; update
wrapLiteLikeValue to first escape backslashes, percent and underscore in the
incoming string (e.g., v = v.replace(/\\/g, "\\\\").replace(/%/g,
"\\%").replace(/_/g, "\\_")), then apply the existing wrapping rules
(%prefix/suffix) based on type, and finally return the escaped+wrapped string
(or pass it through the same escape flow used by wrapLikeValue/SqlString.escape
to remain consistent with the standard analytics path).
- Around line 82-89: getLiteSessionFilter currently builds negated multi-value
clauses with OR which is wrong; update the LITE_FILTER_OPERATORS handling in
getLiteSessionFilter so that for negated string operators (not_equals,
not_contains) when filter.value.length > 1 you join individual column
comparisons with AND instead of OR. Mirror the same semantics in the shared
builder by changing buildStringFilterCondition in
server/src/api/analytics/utils/getFilterStatement.ts to use AND for negated
multi-value (except where user_id semantics differ), and update
wrapLiteLikeValue and wrapLikeValue to escape LIKE metacharacters (% and _) and
append an ESCAPE clause so SqlString.escape is safe for literal matching. Ensure
you reference the operator mapping (LITE_FILTER_OPERATORS), the function
getLiteSessionFilter, the shared buildStringFilterCondition, and the helper
wrapLiteLikeValue/wrapLikeValue when making the changes.
---
Duplicate comments:
In `@server/src/api/analytics/lite/getMetricLite.ts`:
- Around line 36-38: The limit and page query params (variables limit, page, and
offsetStatement in getMetricLite.ts) must be coerced to positive integers and
clamped before use: parse/convert the incoming req.query.limit and
req.query.page to integers (e.g., via parseInt or Math.floor after Number()),
default to 250 and 1 respectively, clamp limit to the range [1, 500] and ensure
page is >=1, and then compute offset using these integer values so OFFSET and
LIMIT are always whole positive numbers; update the offsetStatement construction
to use the sanitized page and limit variables.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0082d658-96b6-45fd-a3fe-cec7185a74f1
📒 Files selected for processing (6)
client/src/app/[site]/components/shared/StandardSection/StandardSection.tsxclient/src/app/[site]/main/page.tsxserver/src/api/analytics/lite/getMetricLite.tsserver/src/api/analytics/lite/getOverviewBucketedLite.tsserver/src/api/analytics/lite/getOverviewLite.tsserver/src/api/analytics/lite/utils.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- client/src/app/[site]/main/page.tsx
Comment on lines
+50
to
+56
| function wrapLiteLikeValue(type: string, value: string | number): string { | ||
| const v = String(value); | ||
| if (type === "contains" || type === "not_contains") return `%${v}%`; | ||
| if (type === "starts_with") return `${v}%`; | ||
| if (type === "ends_with") return `%${v}`; | ||
| return v; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP --type=ts -C4 'starts_with|ends_with|LIKE'Repository: rybbit-io/rybbit
Length of output: 23872
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the main analytics LIKE value wrapper and how it's used
FILE="server/src/api/analytics/utils/getFilterStatement.ts"
echo "=== wrapLikeValue + surrounding ==="
rg -n "wrapLikeValue|buildStringFilterCondition|getSqlParam|LIKE" "$FILE" -n
echo
# Print relevant sections with line numbers
python3 - <<'PY'
import re,sys
path="server/src/api/analytics/utils/getFilterStatement.ts"
with open(path,'r',encoding='utf-8') as f:
lines=f.readlines()
# find wrapLikeValue function start
start=None
for i,l in enumerate(lines):
if 'const wrapLikeValue' in l:
start=i; break
if start is None:
print("wrapLikeValue not found"); sys.exit(1)
for j in range(start, min(start+120, len(lines))):
print(f"{j+1:5d}:{lines[j].rstrip()}")
PY
echo
# Inspect tests about LIKE/wildcards
TEST="server/src/api/analytics/utils/getFilterStatement.test.ts"
echo "=== tests mentioning LIKE wildcards ==="
rg -n "LIKE|wildcards|%\\$\\{v\\}|wrapLikeValue|contains filter" "$TEST"
echo
python3 - <<'PY'
path="server/src/api/analytics/utils/getFilterStatement.test.ts"
with open(path,'r',encoding='utf-8') as f:
lines=f.readlines()
# show around "should handle contains filter with LIKE and wildcards"
for idx,l in enumerate(lines):
if "should handle contains filter with LIKE and wildcards" in l:
start=max(0,idx-20); end=min(len(lines),idx+40)
for j in range(start,end):
print(f"{j+1:5d}:{lines[j].rstrip()}")
break
PYRepository: rybbit-io/rybbit
Length of output: 10006
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="server/src/api/analytics/utils/getFilterStatement.ts"
echo "=== wrapLikeValue + surrounding ==="
python3 - <<'PY'
import sys
path="server/src/api/analytics/utils/getFilterStatement.ts"
with open(path,'r',encoding='utf-8') as f:
lines=f.readlines()
start=None
for i,l in enumerate(lines):
if 'const wrapLikeValue' in l:
start=i; break
if start is None:
print("wrapLikeValue not found"); sys.exit(1)
for j in range(start, min(start+160, len(lines))):
print(f"{j+1:5d}:{lines[j].rstrip()}")
PY
echo
echo "=== occurrences of wrapLikeValue usage ==="
rg -n "wrapLikeValue\\(" "$FILE"
echo
echo "=== tests mentioning LIKE wildcards ==="
TEST="server/src/api/analytics/utils/getFilterStatement.test.ts"
rg -n "contains filter|LIKE|wildcards|entry_page contains" "$TEST"
echo
python3 - <<'PY'
path="server/src/api/analytics/utils/getFilterStatement.test.ts"
with open(path,'r',encoding='utf-8') as f:
lines=f.readlines()
needle="should handle contains filter with LIKE and wildcards"
for idx,l in enumerate(lines):
if needle in l:
start=max(0,idx-15); end=min(len(lines),idx+45)
for j in range(start,end):
print(f"{j+1:5d}:{lines[j].rstrip()}")
break
PYRepository: rybbit-io/rybbit
Length of output: 10295
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="server/src/api/analytics/utils/getFilterStatement.ts"
echo "=== wrapLikeValue + surrounding (approx) ==="
python3 - <<'PY'
path="server/src/api/analytics/utils/getFilterStatement.ts"
with open(path,'r',encoding='utf-8') as f:
lines=f.readlines()
start=None
for i,l in enumerate(lines):
if 'const wrapLikeValue' in l:
start=i; break
if start is None:
print("wrapLikeValue not found"); raise SystemExit(1)
for j in range(start, min(start+160, len(lines))):
print(f"{j+1:5d}:{lines[j].rstrip()}")
PY
echo
echo "=== wrapLikeValue usage ==="
rg -n "wrapLikeValue\\(" "$FILE"
echo
echo "=== tests mentioning LIKE wildcards ==="
TEST="server/src/api/analytics/utils/getFilterStatement.test.ts"
rg -n "LIKE|wildcards|entry_page contains filter|contains filter" "$TEST"
echo
python3 - <<'PY'
path="server/src/api/analytics/utils/getFilterStatement.test.ts"
with open(path,'r',encoding='utf-8') as f:
lines=f.readlines()
needle="should handle contains filter with LIKE and wildcards"
for idx,l in enumerate(lines):
if needle in l:
start=max(0,idx-15); end=min(len(lines),idx+45)
for j in range(start,end):
print(f"{j+1:5d}:{lines[j].rstrip()}")
break
PYRepository: rybbit-io/rybbit
Length of output: 10289
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="server/src/api/analytics/utils/getFilterStatement.ts"
python3 - <<'PY'
path="server/src/api/analytics/utils/getFilterStatement.ts"
with open(path,'r',encoding='utf-8') as f:
lines=f.readlines()
start=None
for i,l in enumerate(lines):
if 'const wrapLikeValue' in l:
start=i; break
if start is None:
print("wrapLikeValue not found"); raise SystemExit(1)
for j in range(start, min(start+170, len(lines))):
print(f"{j+1:5d}:{lines[j].rstrip()}")
PY
echo "WRAP-LIKE-USE:"
rg -n "wrapLikeValue\\(" "$FILE"
TEST="server/src/api/analytics/utils/getFilterStatement.test.ts"
echo "TEST HITS:"
rg -n "wildcards|contains filter with LIKE|entry_page contains filter|LIKE" "$TEST"
python3 - <<'PY'
path="server/src/api/analytics/utils/getFilterStatement.test.ts"
with open(path,'r',encoding='utf-8') as f:
lines=f.readlines()
for idx,l in enumerate(lines):
if "contains filter with LIKE and wildcards" in l:
start=max(0,idx-10); end=min(len(lines),idx+40)
for j in range(start,end):
print(f"{j+1:5d}:{lines[j].rstrip()}")
break
PYRepository: rybbit-io/rybbit
Length of output: 9894
Escape LIKE wildcard metacharacters in LIKE filter values
wrapLiteLikeValue wraps the raw input with % (e.g., %${v}%, ${v}%, %${v}) without escaping %/_ (and \). This means user values containing LIKE wildcards will over-match. The standard analytics path (server/src/api/analytics/utils/getFilterStatement.ts) uses the same LIKE wrapping logic (via wrapLikeValue(...) + SqlString.escape(...)), so the issue isn’t lite-specific.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/src/api/analytics/lite/utils.ts` around lines 50 - 56,
wrapLiteLikeValue currently inserts % wildcards around raw input without
escaping LIKE metacharacters, causing overmatching; update wrapLiteLikeValue to
first escape backslashes, percent and underscore in the incoming string (e.g., v
= v.replace(/\\/g, "\\\\").replace(/%/g, "\\%").replace(/_/g, "\\_")), then
apply the existing wrapping rules (%prefix/suffix) based on type, and finally
return the escaped+wrapped string (or pass it through the same escape flow used
by wrapLikeValue/SqlString.escape to remain consistent with the standard
analytics path).
Comment on lines
+82
to
+89
| const op = LITE_FILTER_OPERATORS[filter.type]; | ||
| if (!op || filter.value.length === 0) return { supported: false, sql: "" }; | ||
|
|
||
| const parts = filter.value.map( | ||
| value => `${column} ${op} ${SqlString.escape(wrapLiteLikeValue(filter.type, value))}` | ||
| ); | ||
| conditions.push(parts.length === 1 ? parts[0] : `(${parts.join(" OR ")})`); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find standard filter-statement builder and inspect join semantics for negated ops.
rg -nP --type=ts -C6 'not_contains|not_equals'Repository: rybbit-io/rybbit
Length of output: 49212
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect lite getLiteSessionFilter join semantics
echo "---- lite: server/src/api/analytics/lite/utils.ts (getLiteSessionFilter) ----"
rg -n "function getLiteSessionFilter|getLiteSessionFilter" server/src/api/analytics/lite/utils.ts
python3 - <<'PY'
import itertools,sys,os, pathlib, re
path="server/src/api/analytics/lite/utils.ts"
with open(path,'r',encoding='utf-8') as f:
lines=f.readlines()
for i,l in enumerate(lines, start=1):
if "getLiteSessionFilter" in l or "wrapLiteLikeValue" in l or "LITE_FILTER_OPERATORS" in l:
pass
start=70; end=100
for i in range(start,end+1):
print(f"{i}:{lines[i-1].rstrip()}")
PY
# 2) Inspect standard getFilterStatement implementation for join semantics
echo "---- standard: server/src/api/analytics/utils/getFilterStatement.ts (string filter condition builder) ----"
python3 - <<'PY'
path="server/src/api/analytics/utils/getFilterStatement.ts"
with open(path,'r',encoding='utf-8') as f:
lines=f.readlines()
# Print around buildStringFilterCondition and any join logic near not_equals/not_contains
for i in range(130,230):
if "joiner" in lines[i-1] or "not_equals" in lines[i-1] or "not_contains" in lines[i-1] or "buildStringFilterCondition" in lines[i-1]:
pass
start=250; end=380
for i in range(start,end+1):
print(f"{i}:{lines[i-1].rstrip()}")
PY
# 3) Inspect bots builder joiner (already seen in grep, but show exact function)
echo "---- bots: server/src/api/analytics/bots/utils.ts (buildStringFilterCondition) ----"
python3 - <<'PY'
path="server/src/api/analytics/bots/utils.ts"
with open(path,'r',encoding='utf-8') as f:
lines=f.readlines()
start=120; end=180
for i in range(start,end+1):
print(f"{i}:{lines[i-1].rstrip()}")
PY
# 4) Search tests for LIKE wildcard escaping expectations
echo "---- tests: LIKE wildcard coverage in getFilterStatement.test.ts ----"
rg -n "LIKE|not_contains|contains" server/src/api/analytics/utils/getFilterStatement.test.ts
rg -n "'%|\"%|_'|\\_%|\\\\_|wildcard" server/src/api/analytics/utils/getFilterStatement.test.ts || trueRepository: rybbit-io/rybbit
Length of output: 7290
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- lite: server/src/api/analytics/lite/utils.ts (wrapLiteLikeValue + getLiteSessionFilter) ----"
nl -ba server/src/api/analytics/lite/utils.ts | sed -n '45,105p'
echo "---- standard: find buildStringFilterCondition in getFilterStatement.ts ----"
rg -n "buildStringFilterCondition" server/src/api/analytics/utils/getFilterStatement.ts
echo "---- standard: server/src/api/analytics/utils/getFilterStatement.ts (buildStringFilterCondition body) ----"
# Show around the function definition (best-effort with a window)
python3 - <<'PY'
import re
path="server/src/api/analytics/utils/getFilterStatement.ts"
lines=open(path,'r',encoding='utf-8').read().splitlines()
for i,l in enumerate(lines):
if "function buildStringFilterCondition" in l:
start=max(0,i-5); end=min(len(lines), i+120)
for j in range(start,end):
print(f"{j+1}:{lines[j]}")
break
else:
# fallback: show from first occurrence of "const buildStringFilterCondition"
for i,l in enumerate(lines):
if "buildStringFilterCondition" in l and ("const " in l or "export" in l):
start=max(0,i-5); end=min(len(lines), i+120)
for j in range(start,end):
print(f"{j+1}:{lines[j]}")
break
PY
echo "---- standard tests around LIKE + (not_)contains ----"
nl -ba server/src/api/analytics/utils/getFilterStatement.test.ts | sed -n '120,190p'
echo "---- search tests for escaping of LIKE metacharacters (% and _) ----"
rg -n "escape|wildcard|LIKE|%|_|not_contains|contains" server/src/api/analytics/utils/getFilterStatement.test.ts -S | head -n 200Repository: rybbit-io/rybbit
Length of output: 219
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- lite: wrapLiteLikeValue + getLiteSessionFilter ----"
python3 - <<'PY'
path="server/src/api/analytics/lite/utils.ts"
lines=open(path,'r',encoding='utf-8').read().splitlines()
for i,l in enumerate(lines, start=1):
if "function wrapLiteLikeValue" in l:
start=i-10; end=i+40
break
else:
start=1; end=120
for j in range(max(1,start), min(len(lines),end)+1):
print(f"{j}:{lines[j-1]}")
print("---- getLiteSessionFilter ----")
for i,l in enumerate(lines, start=1):
if "export function getLiteSessionFilter" in l:
start=i-5; end=i+60
for j in range(max(1,start), min(len(lines),end)+1):
print(f"{j}:{lines[j-1]}")
break
PY
echo "---- standard: buildStringFilterCondition implementation ----"
python3 - <<'PY'
path="server/src/api/analytics/utils/getFilterStatement.ts"
lines=open(path,'r',encoding='utf-8').read().splitlines()
idx=None
for i,l in enumerate(lines, start=1):
if "buildStringFilterCondition" in l and "function" in l:
idx=i; break
# fallback to const
if idx is None:
for i,l in enumerate(lines, start=1):
if "const buildStringFilterCondition" in l:
idx=i; break
if idx is None:
raise SystemExit("Could not locate buildStringFilterCondition")
start=idx-10; end=idx+120
for j in range(max(1,start), min(len(lines),end)+1):
print(f"{j}:{lines[j-1]}")
PY
echo "---- standard tests: getFilterStatement.test.ts (contains/not_contains + LIKE) ----"
python3 - <<'PY'
import re
path="server/src/api/analytics/utils/getFilterStatement.test.ts"
lines=open(path,'r',encoding='utf-8').read().splitlines()
# print tests containing not_contains/contains and any LIKE expectations
pat=re.compile(r"not_contains|contains.*LIKE|LIKE.*%|pathname LIKE|NOT LIKE")
hits=[i for i,l in enumerate(lines, start=1) if pat.search(l)]
for i in hits[:80]:
start=max(1,i-10); end=min(len(lines), i+10)
print(f"\n--- around line {i} ---")
for j in range(start,end+1):
print(f"{j}:{lines[j-1]}")
PY
echo "---- standard tests: search for LIKE metacharacter escaping (% or _) ----"
rg -n --hidden -S "%'|LIKE '%|LIKE \"%|NOT LIKE|\\_%|\\\\_%|\\_|wildcard|escape" server/src/api/analytics/utils/getFilterStatement.test.ts || trueRepository: rybbit-io/rybbit
Length of output: 17518
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- find lite session filter usage ----"
rg -n "getLiteSessionFilter\(" server/src/api/analytics/lite -S
echo "---- find unit tests for lite utils ----"
rg -n "lite/utils|getLiteSessionFilter|wrapLiteLikeValue" server/src -S
echo "---- search standard builder tests for multiple not_contains ----"
rg -n "not_contains.*value: \\[.*,.+\\]" server/src/api/analytics/utils/getFilterStatement.test.ts -S || true
echo "---- search standard builder tests for multiple not_equals ----"
rg -n "not_equals.*value: \\[.*,.+\\]" server/src/api/analytics/utils/getFilterStatement.test.ts -S || trueRepository: rybbit-io/rybbit
Length of output: 2197
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- lite: LITE_SESSION_FILTER_COLUMNS mapping ----"
python3 - <<'PY'
path="server/src/api/analytics/lite/utils.ts"
lines=open(path,'r',encoding='utf-8').read().splitlines()
for i,l in enumerate(lines, start=1):
if "const LITE_SESSION_FILTER_COLUMNS" in l:
start=i-2
end=i+60
break
else:
raise SystemExit("mapping not found")
for j in range(start,end+1):
print(f"{j}:{lines[j-1]}")
PY
echo "---- standard tests: look for any mention of literal '%' or '_' in expected LIKE strings ----"
rg -n "LIKE '(%|.*%|.*_).*'|NOT LIKE '(%|.*%|.*_).*'|LIKE \"%|NOT LIKE \"%|NOT LIKE '%|LIKE '%|LIKE .*_.*|NOT LIKE .*_.*" server/src/api/analytics/utils/getFilterStatement.test.ts -S || true
echo "---- standard tests: look for patterns that include underscores in LIKE expectations ----"
rg -n "LIKE '.*_.*'|NOT LIKE '.*_.*'" server/src/api/analytics/utils/getFilterStatement.test.ts -S || true
echo "---- standard builder: confirm wrapLikeValue does not escape LIKE metacharacters ----"
python3 - <<'PY'
path="server/src/api/analytics/utils/getFilterStatement.ts"
lines=open(path,'r',encoding='utf-8').read().splitlines()
# print around wrapLikeValue
for i,l in enumerate(lines, start=1):
if "const wrapLikeValue" in l:
start=i-5; end=i+30
break
else:
raise SystemExit("wrapLikeValue not found")
for j in range(start,end+1):
print(f"{j}:{lines[j-1]}")
PYRepository: rybbit-io/rybbit
Length of output: 4414
Fix negated multi-value filters in lite (must AND, not OR).
getLiteSessionFilter currently builds negated multi-value clauses with OR (e.g. col != 'a' OR col != 'b' / col NOT LIKE ... OR ...), which is always true and effectively disables the filter for not_equals and not_contains.
- Update the lite join semantics to use
ANDfornot_equals/not_containswhenfilter.value.length > 1. - The raw analytics builder (
server/src/api/analytics/utils/getFilterStatement.ts,buildStringFilterCondition) also usesjoin(" OR ")for multi-value negated filters for non-user_idparameters (unit test asserts this fornot_equals), so changing lite alone would create lite/raw inconsistencies—fix the shared semantics too. - Minor:
wrapLiteLikeValue(and the standardwrapLikeValue) inserts%but only usesSqlString.escape(quotes);%/_in user values will behave as LIKE wildcards. Escape LIKE metacharacters (and addESCAPE) if literal matching is intended.
🐛 Proposed fix
const op = LITE_FILTER_OPERATORS[filter.type];
if (!op || filter.value.length === 0) return { supported: false, sql: "" };
+ const negated = filter.type === "not_equals" || filter.type === "not_contains";
+ const joiner = negated ? " AND " : " OR ";
const parts = filter.value.map(
value => `${column} ${op} ${SqlString.escape(wrapLiteLikeValue(filter.type, value))}`
);
- conditions.push(parts.length === 1 ? parts[0] : `(${parts.join(" OR ")})`);
+ conditions.push(parts.length === 1 ? parts[0] : `(${parts.join(joiner)})`);📝 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
| const op = LITE_FILTER_OPERATORS[filter.type]; | |
| if (!op || filter.value.length === 0) return { supported: false, sql: "" }; | |
| const parts = filter.value.map( | |
| value => `${column} ${op} ${SqlString.escape(wrapLiteLikeValue(filter.type, value))}` | |
| ); | |
| conditions.push(parts.length === 1 ? parts[0] : `(${parts.join(" OR ")})`); | |
| } | |
| const op = LITE_FILTER_OPERATORS[filter.type]; | |
| if (!op || filter.value.length === 0) return { supported: false, sql: "" }; | |
| const negated = filter.type === "not_equals" || filter.type === "not_contains"; | |
| const joiner = negated ? " AND " : " OR "; | |
| const parts = filter.value.map( | |
| value => `${column} ${op} ${SqlString.escape(wrapLiteLikeValue(filter.type, value))}` | |
| ); | |
| conditions.push(parts.length === 1 ? parts[0] : `(${parts.join(joiner)})`); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/src/api/analytics/lite/utils.ts` around lines 82 - 89,
getLiteSessionFilter currently builds negated multi-value clauses with OR which
is wrong; update the LITE_FILTER_OPERATORS handling in getLiteSessionFilter so
that for negated string operators (not_equals, not_contains) when
filter.value.length > 1 you join individual column comparisons with AND instead
of OR. Mirror the same semantics in the shared builder by changing
buildStringFilterCondition in
server/src/api/analytics/utils/getFilterStatement.ts to use AND for negated
multi-value (except where user_id semantics differ), and update
wrapLiteLikeValue and wrapLikeValue to escape LIKE metacharacters (% and _) and
append an ESCAPE clause so SqlString.escape is safe for literal matching. Ensure
you reference the operator mapping (LITE_FILTER_OPERATORS), the function
getLiteSessionFilter, the shared buildStringFilterCondition, and the helper
wrapLiteLikeValue/wrapLikeValue when making the changes.
- Removed unused admin routes from the Stripe admin routes section to streamline the API. - Cleaned up the getMetricLite function by removing outdated comments and unnecessary code, enhancing readability. - Updated ClickHouse materialized views and backfill scripts to exclude session attributes that are not session-invariant, ensuring data integrity.
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
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
Release Notes
New Features
Improvements
Documentation