Open
Conversation
Contributor
📝 WalkthroughWalkthroughAdds first-class "groups" across the stack: SDK, validation, API, buffers, DB schema and services, TRPC router, worker cron/flush, frontend UI/pages/modals/tables, importers, and a demo testbed—enabling group CRUD, membership, and group-affiliated analytics ingestion and queries. Changes
Sequence Diagram(s)sequenceDiagram
participant SDK as Client SDK
participant API as Track API
participant Redis as Redis Buffer (groupBuffer & event/profile buffers)
participant Worker as Worker Cron
participant CH as ClickHouse
SDK->>API: track(payload + groups)
API->>Redis: enqueue event / group add (store in Redis list & cache)
Redis-->>API: ack
Note over Redis,Worker: buffered entries persisted in Redis with per-item cache
Worker->>Redis: cron flushGroups -> pop batch
Worker->>CH: bulk insert JSONEachRow (groups/events/profiles)
CH-->>Worker: insert result
Worker->>Redis: cleanup processed buffer entries
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: 1
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)
apps/start/src/components/report-chart/funnel/chart.tsx (1)
110-116:⚠️ Potential issue | 🟡 MinorMissing
keyon Fragment in.map()iteration.The
keyprop is placed on the inner<span>instead of the outermost element. When iterating with.map(), React requires the key on the outermost element. Since shorthand fragments (<>) cannot accept keys, use the explicitFragmentsyntax.Proposed fix
+import { Fragment } from 'react';{breakdowns.map((name, index) => { return ( - <> + <Fragment key={name}> {index !== 0 && <ChevronRightIcon className="size-3" />} - <span key={name}>{name}</span> - </> + <span>{name}</span> + </Fragment> ); })}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/start/src/components/report-chart/funnel/chart.tsx` around lines 110 - 116, In the breakdowns.map rendering, the key is incorrectly placed on the inner <span> instead of the outer fragment; update the mapped return so the outer wrapper accepts the key by replacing the shorthand fragment (<>) with React.Fragment (imported if needed) or another wrapper element and move the key prop to that outer Fragment (i.e., apply the key to the Fragment returned by breakdowns.map that currently contains ChevronRightIcon and <span>), keeping the existing ChevronRightIcon and <span> content unchanged.apps/start/src/components/report/sidebar/PropertiesCombobox.tsx (1)
113-132:⚠️ Potential issue | 🟠 MajorUse
valueas the virtual-list key for the event/profile rows.Lines 210 and 249 use
itemKey="id", but the corresponding row objects only containvalue,label, anddescription. This causesrc-virtual-listto key rows asundefined, which will reuse the wrong row after filtering or during animation. Line 289 (group list) already uses the correctitemKey="value".🛠️ Possible fix
<VirtualList data={filteredActions} height={300} itemHeight={40} - itemKey="id" + itemKey="value" ><VirtualList data={filteredActions} height={300} itemHeight={40} - itemKey="id" + itemKey="value" >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/start/src/components/report/sidebar/PropertiesCombobox.tsx` around lines 113 - 132, The virtual-list rows for profileActions and eventActions are keyed with itemKey="id" but the row objects only have value/label/description, causing undefined keys; update the rc-virtual-list usage for the lists that render profileActions and eventActions to use itemKey="value" (matching the objects' value property) so rows are keyed correctly—the group list already uses itemKey="value", mirror that for the profileActions and eventActions lists.packages/db/src/services/import.service.ts (1)
449-466:⚠️ Potential issue | 🟠 MajorMissing
groupscolumn in migration query and staging table schema.The
moveImportsToProductionfunction copies events from staging to production without including thegroupscolumn in SELECT/INSERT statements. Additionally, theevents_importsstaging table itself lacks thegroupscolumn added to the productioneventstable in migration 11. Events with groups set at lines 358 and 394 will be lost during migration.Also note: This query should use the custom query builder from
./packages/db/src/clickhouse/query-builder.tsinstead of raw SQL, per coding guidelines.🛠️ Proposed fixes
- Add
groupscolumn toevents_importstable schema in migration 5:columns: [ // ... existing columns ... '`session_id` String CODEC(LZ4)', + '`groups` Array(String) DEFAULT [] CODEC(ZSTD(3))', '`path` String CODEC(ZSTD(3))',
- Include
groupsin the migration query:const migrationQuery = ` INSERT INTO ${TABLE_NAMES.events} ( id, name, sdk_name, sdk_version, device_id, profile_id, project_id, - session_id, path, origin, referrer, referrer_name, referrer_type, + session_id, groups, path, origin, referrer, referrer_name, referrer_type, duration, properties, created_at, country, city, region, longitude, latitude, os, os_version, browser, browser_version, device, brand, model, imported_at ) SELECT id, name, sdk_name, sdk_version, device_id, profile_id, project_id, - session_id, path, origin, referrer, referrer_name, referrer_type, + session_id, groups, path, origin, referrer, referrer_name, referrer_type, duration, properties, created_at, country, city, region, longitude, latitude, os, os_version, browser, browser_version, device, brand, model, imported_at FROM ${TABLE_NAMES.events_imports} WHERE ${whereClause} ORDER BY created_at ASC `;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/src/services/import.service.ts` around lines 449 - 466, The migration is dropping the `groups` data because the staging table and migration SQL in moveImportsToProduction don't include it: update the events_imports staging table schema (migration that creates/defines events_imports) to add the groups column with the same type/default as the production events table, then add `groups` to both the INSERT column list and the SELECT list in the migrationQuery used in moveImportsToProduction (references: migrationQuery, moveImportsToProduction, TABLE_NAMES.events, TABLE_NAMES.events_imports). Finally replace the raw SQL construction for migrationQuery with the project’s clickhouse query builder (from query-builder.ts) so the query follows coding guidelines.packages/db/src/services/session.service.ts (1)
217-246: 🛠️ Refactor suggestion | 🟠 MajorPlease stop extending this ClickHouse path via
createSqlBuilder()and raw SQL fragments.This PR adds
groupsto a query that is still assembled outside the repo’s required ClickHouse builder. Since this path is already being modified, it should be migrated toquery-builder.ts/query-functions.tsinstead of deepening the parallel abstraction.As per coding guidelines, "When writing ClickHouse queries, always use the custom query builder from
./packages/db/src/clickhouse/query-builder.tsand./packages/db/src/clickhouse/query-functions.ts".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/src/services/session.service.ts` around lines 217 - 246, The current code appends raw ClickHouse fragments (the columns array, sb.select assignments including sb.select.has_replay, and sb.joins.has_replay with TABLE_NAMES.session_replay_chunks) directly via createSqlBuilder(), which violates the project's ClickHouse builder usage; refactor by moving the new "groups" column and the has_replay logic into the clickhouse query builder utilities (query-builder.ts / query-functions.ts): add a query-function that returns the hasReplay select expression and required LEFT JOIN (parameterized by projectId and dateIntervalInDays) and update the place where columns are built to use the builder's select helper instead of mutating sb.select/sb.joins directly (refer to symbols: columns, sb.select, sb.joins, has_replay, groups, TABLE_NAMES.session_replay_chunks, createSqlBuilder).packages/db/src/services/profile.service.ts (1)
121-133:⚠️ Potential issue | 🟡 MinorMissing
groupsfield ingetProfileByIdquery.The
getProfilesfunction (lines 168-169) includesany(groups) as groupsin the SELECT, butgetProfileByIddoes not fetch the groups field. This inconsistency means profiles fetched by ID won't have their groups populated.🐛 Proposed fix to add groups to query
const [profile] = await chQuery<IClickhouseProfile>( `SELECT id, project_id, last_value(nullIf(first_name, '')) as first_name, last_value(nullIf(last_name, '')) as last_name, last_value(nullIf(email, '')) as email, last_value(nullIf(avatar, '')) as avatar, last_value(is_external) as is_external, last_value(properties) as properties, - last_value(created_at) as created_at + last_value(created_at) as created_at, + any(groups) as groups FROM ${TABLE_NAMES.profiles} FINAL WHERE id = ${sqlstring.escape(String(id))} AND project_id = ${sqlstring.escape(projectId)} GROUP BY id, project_id ORDER BY created_at DESC LIMIT 1` );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/src/services/profile.service.ts` around lines 121 - 133, The getProfileById query is missing the groups field, causing inconsistent profile objects; update the SELECT inside the chQuery used in getProfileById (the const [profile] = await chQuery<IClickhouseProfile>(...)) to include groups — e.g., add any(groups) as groups (matching getProfiles) in the SELECT list so the returned profile has its groups populated. Ensure the field name matches IClickhouseProfile and the GROUP BY / aggregation style remains consistent with the existing query.packages/trpc/src/routers/chart.ts (1)
236-320:⚠️ Potential issue | 🟠 MajorExpose
group.*keys fromproperties()as well.
values()now resolvesgroup.*, but this endpoint still only returns event and profile keys. Withoutgroup.id/group.name/group.typeplus dynamicgroup.properties.*, the new group breakdown/filter support is not discoverable in the report builder.🛠️ Suggested fix
import { AggregateChartEngine, ChartEngine, ch, chQuery, clix, conversionService, createSqlBuilder, formatClickhouseDate, funnelService, getChartPrevStartEndDate, getChartStartEndDate, getEventFiltersWhereClause, getEventMetasCached, + getGroupPropertyKeys, getProfilesCached, getReportById, getSelectPropertyKey, getSettingsForProject, getProfilePropertySelect, getGroupPropertySelect, type IServiceProfile, onlyReportEvents, sankeyService, TABLE_NAMES, validateShareAccess, } from '@openpanel/db';- const profiles = await clix(ch, 'UTC') - .select<Pick<IServiceProfile, 'properties'>>(['properties']) - .from(TABLE_NAMES.profiles) - .where('project_id', '=', projectId) - .where('is_external', '=', true) - .limit(10_000) - .execute(); + const [profiles, groupPropertyKeys] = await Promise.all([ + clix(ch, 'UTC') + .select<Pick<IServiceProfile, 'properties'>>(['properties']) + .from(TABLE_NAMES.profiles) + .where('project_id', '=', projectId) + .where('is_external', '=', true) + .limit(10_000) + .execute(), + getGroupPropertyKeys(projectId), + ]); ... + const groupProperties = [ + 'group.id', + 'group.name', + 'group.type', + ...groupPropertyKeys.map((k) => `group.properties.${k}`), + ]; ... const properties = [ ...eventProperties, ...(event === '*' || !event ? ['name'] : []), ...fixedProperties, ...profileProperties, + ...groupProperties, ];
🟠 Major comments (20)
packages/common/scripts/get-referrers.ts-128-130 (1)
128-130:⚠️ Potential issue | 🟠 MajorFail hard when the download step breaks.
A 4xx/5xx response is treated like success here, and the
catchonly logs the error. That makes generator failures easy to miss and can leavereferrers/index.tsstale while the script still exits 0.Suggested fix
- const data = await fetch( - 'https://s3-eu-west-1.amazonaws.com/snowplow-hosted-assets/third-party/referer-parser/referers-latest.json' - ).then((res) => res.json()); + const res = await fetch( + 'https://s3-eu-west-1.amazonaws.com/snowplow-hosted-assets/third-party/referer-parser/referers-latest.json' + ); + if (!res.ok) { + throw new Error(`Failed to download referers: ${res.status} ${res.statusText}`); + } + const data = await res.json(); @@ - } catch (e) { - console.log(e); + } catch (e) { + console.error(e); + process.exitCode = 1; }Also applies to: 150-152
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/common/scripts/get-referrers.ts` around lines 128 - 130, The fetch call that assigns to the variable "data" in get-referrers.ts currently treats any HTTP response as success; update the fetch handling so it checks res.ok and throws an error (including status and statusText) when the response is not OK, causing the script to fail non‑zero instead of continuing; do the same for the other fetch at the second occurrence (lines around the other fetch/then block) so failed downloads surface and stop generation of referrers/index.ts.packages/common/scripts/get-referrers.ts-77-77 (1)
77-77:⚠️ Potential issue | 🟠 MajorDon't remap
bing.comtoai.This table is hostname-only and is merged after Snowplow's dataset, so this entry will relabel all regular Bing search referrals as AI traffic. That will skew source analytics pretty badly. If you want Copilot-specific attribution, it needs an AI-specific host; otherwise
bing.comshould stay in its upstream category.Suggested fix
- 'bing.com': { type: 'ai', name: 'Bing AI' },Also applies to: 141-144
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/common/scripts/get-referrers.ts` at line 77, The 'bing.com' entry in the hostname mapping is incorrectly remapping all Bing traffic to AI; locate the hostname-to-category map that contains the 'bing.com' key (the object with entries like 'bing.com': { type: 'ai', name: 'Bing AI' }) and revert this entry to its upstream category or remove it entirely so regular bing.com referrals keep their original classification; if you need Copilot-specific attribution instead, add a separate entry that matches the actual Copilot-specific host (not plain bing.com).docker-compose.yml-25-25 (1)
25-25:⚠️ Potential issue | 🟠 MajorAdd migration guidance for ClickHouse data volume when upgrading versions.
This bumps
op-chfrom25.10.2.65to26.1.3.52, moving from a version near the 25.8 LTS line to the 26.1 regular release line. The container reuses./docker/data/op-ch-data, which persists across restarts. ClickHouse issue#86837documents a critical compatibility issue: v25.8+ introduced a new compact data part format (marks-per-substream), and data directories upgraded to this format cannot be downgraded or safely moved between incompatible versions without data loss. Contributors switching branches will encounter container startup failures if the local volume was previously upgraded. Add a note in the contributing docs or docker setup guide instructing developers to reset the local volume when switching ClickHouse versions, or back it up before upgrading.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-compose.yml` at line 25, The ClickHouse image version bump (image: clickhouse/clickhouse-server:26.1.3.52) can cause incompatible on-disk data in the persisted volume ./docker/data/op-ch-data; add a short migration note to the contributing/docs or docker setup guide that instructs developers to back up the existing ./docker/data/op-ch-data before upgrading, and to reset/clear that local volume (or recreate it) when switching between ClickHouse major/minor lines (e.g., 25.10.x → 26.1.x) to avoid the marks-per-substream incompatibility and startup failures.packages/validation/src/track.validation.ts-10-10 (1)
10-10:⚠️ Potential issue | 🟠 MajorReject empty identifiers in the new group fields.
Line 10 and Line 18 both accept
""today, so a blankprofileIdor group entry passes validation and can be stored as a real association.Suggested change
- profileId: z.string().optional(), + profileId: z.string().min(1).optional(),- groups: z.array(z.string()).optional(), + groups: z.array(z.string().min(1)).optional(),Also applies to: 18-18
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/validation/src/track.validation.ts` at line 10, profileId and the group entry validator currently allow empty strings; change their Zod string schemas to forbid empty values by using z.string().nonempty().optional() for profileId and replace the group item validator (the string schema used for group entries) with z.string().nonempty() (or z.string().nonempty().optional() if the group item can be omitted) so empty "" values are rejected during validation.apps/start/src/components/report/sidebar/PropertiesCombobox.tsx-213-223 (1)
213-223:⚠️ Potential issue | 🟠 MajorUse semantic button elements for keyboard accessibility in dropdown options.
The motion.div elements at lines 213-223, 252-262, and 290-301 lack keyboard focusability and event handlers, making them inaccessible to keyboard-only users. Convert them to
motion.button(or wrap inDropdownMenuItem asChild) to support Tab navigation and Enter/Space activation, matching the semantics used in the top-level menu (renderIndex).Suggested fix
- <motion.div + <motion.button + type="button" animate={{ opacity: 1, y: 0 }} - className="col cursor-pointer gap-px rounded-md p-2 hover:bg-accent" + className="col w-full gap-px rounded-md p-2 text-left hover:bg-accent" initial={{ opacity: 0, y: 10 }} onClick={() => handleSelect(action)} > <div className="font-medium">{action.label}</div> <div className="text-muted-foreground text-sm"> {action.description} </div> - </motion.div> + </motion.button>Apply to
renderEvent,renderProfile, andrenderGroup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/start/src/components/report/sidebar/PropertiesCombobox.tsx` around lines 213 - 223, The dropdown option elements rendered by renderEvent, renderProfile, and renderGroup use motion.div which is not keyboard-focusable; replace the motion.div wrappers with motion.button (or use DropdownMenuItem with asChild) so options are reachable by Tab and activatable via Enter/Space, preserving existing props (animate, initial, className, onClick which calls handleSelect) and contents (action.label and action.description) and keeping any styling and motion behavior intact.packages/db/src/buffers/profile-buffer.test.ts-38-47 (1)
38-47:⚠️ Potential issue | 🟠 MajorReplace
flushdb()andquit()with pattern-based cleanup of owned keys only.Since
getRedisCache()is a process-wide singleton,flushdb()erases unrelated test data andquit()breaks the connection for concurrent suites. Instead, useredis.keys('profile-*')to identify only keys owned by this suite, clean them withredis.del()inbeforeEachandafterEach, and remove theafterAllblock. Seepackages/redis/cachable.test.tsfor the established pattern in your codebase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/src/buffers/profile-buffer.test.ts` around lines 38 - 47, Replace the global flushdb()/quit() usage with pattern-based key cleanup: in the beforeEach hook (where redis.flushdb() is called) use redis.keys('profile-*') and delete those keys via redis.del(), and remove the afterAll block that quits the client; add an afterEach hook that also deletes redis.keys('profile-*') to ensure per-test cleanup; keep vi.mocked(chQuery).mockResolvedValue([]) as-is and do not call redis.quit() anywhere in this test file to avoid closing the process-wide getRedisCache() connection.packages/db/src/clickhouse/query-builder.ts-424-427 (1)
424-427:⚠️ Potential issue | 🟠 Major
rawJoin()loses join order, breakingARRAY JOINplacement.ClickHouse requires
ARRAY JOINto appear immediately after the source table in the FROM clause, before other joins. The current implementation unconditionally emits all structured joins first, then appends all raw joins, regardless of call order. This placesARRAY JOINafterLEFT JOINand similar clauses, violating ClickHouse semantics and causing incorrect query behavior.Use a single ordered join list or a dedicated method (e.g.,
arrayJoin()) to preserve insertion order.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/src/clickhouse/query-builder.ts` around lines 424 - 427, The fix is to preserve call order for joins instead of emitting structured joins first then appending this._rawJoins; update rawJoin() and the join emission logic so all joins are stored in a single ordered list (e.g., merge _rawJoins into the same join list or push an object into this._joins with a type flag) and then iterate that unified list when building parts (replace the separate this._rawJoins.forEach(...) emit with a single ordered emit of this._joins), or alternatively add a dedicated arrayJoin() that inserts its entry at the correct index in the primary join list; make sure to reference and update rawJoin(), this._rawJoins, this._joins and the parts.push(join) emission site so ARRAY JOIN appears in the final SQL exactly in call order.packages/db/src/buffers/profile-buffer.ts-87-91 (1)
87-91:⚠️ Potential issue | 🟠 MajorProceeding without lock on failure may cause data races.
When lock acquisition fails, the code logs an error but proceeds to execute the function anyway. This defeats the purpose of locking and could lead to lost profile updates under high contention. Consider either throwing an error or implementing a more robust fallback strategy.
🛡️ Proposed fix to fail explicitly
this.logger.error('Failed to acquire profile lock, proceeding without lock', { profileId, projectId, }); - return fn(); + throw new Error(`Failed to acquire profile lock for ${projectId}:${profileId} after ${maxRetries} retries`);Alternatively, if you need to proceed, consider adding a metric/alert for monitoring lock acquisition failures to detect contention issues in production.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/src/buffers/profile-buffer.ts` around lines 87 - 91, The code in the profile lock path (inside the method that logs "Failed to acquire profile lock, proceeding without lock" and then calls fn()) must not silently proceed on lock acquisition failure; update the logic in that method (e.g., acquireProfileLock / withProfileLock where fn is invoked) to either throw an explicit error when the lock cannot be acquired or implement a safe fallback: add a short retry with exponential backoff and only call fn() after successful lock acquisition, and if still failing, throw. Also emit a metric/alert (e.g., this.metrics.increment or this.logger.warn with a unique metric key like "profile_lock_acquire_fail") including profileId and projectId so production contention is monitored.packages/db/src/buffers/group-buffer.ts-90-124 (1)
90-124:⚠️ Potential issue | 🟠 MajorAdd distributed locking to prevent race conditions in concurrent group updates.
The
addmethod performs a read-modify-write pattern (fetch existing → merge properties → write) without any locking mechanism. UnlikeProfileBufferwhich wraps its logic inwithProfileLock,GroupBufferis vulnerable to lost updates when multiple concurrent requests update the same group. This occurs inapps/api/src/controllers/track.controller.tswheregroupBuffer.add()is called withinPromise.all(), allowing concurrent execution.Implement a similar distributed lock pattern using Redis SET with NX flag and retry logic, as demonstrated in
ProfileBuffer.withProfileLock().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/src/buffers/group-buffer.ts` around lines 90 - 124, The add method on GroupBuffer must be wrapped in a distributed lock (similar to ProfileBuffer.withProfileLock) to prevent concurrent read-modify-write races: implement a withGroupLock (or inline lock) that uses Redis SET with NX and a short TTL, retries acquiring the lock with backoff, executes the existing fetchFromCache/fetchFromClickhouse → merge → write logic only while holding the lock, and always releases the lock (delete) in a finally block; reference GroupBuffer.add, ProfileBuffer.withProfileLock, fetchFromCache, fetchFromClickhouse, and toDots when adding the lock and retry logic so concurrent groupBuffer.add calls for the same projectId+id are serialized.packages/trpc/src/routers/group.ts-96-114 (1)
96-114:⚠️ Potential issue | 🟠 MajorUse the custom query builder instead of raw SQL queries.
The
metricsendpoint and several others (activity, members, mostEvents, popularRoutes, memberGrowth) use rawchQuerycalls with string templates. Per coding guidelines, ClickHouse queries must use the custom query builder from./packages/db/src/clickhouse/query-builder.ts. The query builder supports these aggregation functions—they are passed as string expressions in theselect()method (e.g.,query.select(['count() AS totalEvents', 'uniqExact(profile_id) AS uniqueProfiles'])).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/trpc/src/routers/group.ts` around lines 96 - 114, The metrics endpoint (and other endpoints like activity, members, mostEvents, popularRoutes, memberGrowth) currently embeds raw SQL strings in chQuery; replace that with the project's ClickHouse query builder (from query-builder.ts) by constructing a builder-based query: call query.select(['count() AS totalEvents', 'uniqExact(profile_id) AS uniqueProfiles', 'min(created_at) AS firstSeen', 'max(created_at) AS lastSeen']).from(TABLE_NAMES.events) and add equivalent where clauses that safely inject projectId and id (use the builder's where/whereRaw API or sqlstring.escape inside builder where expressions) and then execute via the builder's run/execute method instead of chQuery; update the metrics resolver to use the builder API (reference the metrics protectedProcedure and chQuery usage) so aggregation expressions are passed as string items to select() per guidelines.packages/db/src/services/event.service.ts-617-619 (1)
617-619:⚠️ Potential issue | 🟠 MajorCarry the new
groupIdfilter intogetEventsCount().
getEventList()applieshas(groups, ...), but the count path never readsgroupId, so group pages paginate against the unfiltered total.🛠️ Suggested fix
export async function getEventsCount({ projectId, profileId, + groupId, events, filters, startDate, endDate, }: Omit<GetEventListOptions, 'cursor' | 'take'>) { ... if (profileId) { sb.where.profileId = `profile_id = ${sqlstring.escape(profileId)}`; } + + if (groupId) { + sb.where.groupId = `has(groups, ${sqlstring.escape(groupId)})`; + }Also applies to: 681-709
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/src/services/event.service.ts` around lines 617 - 619, getEventList applies a group filter via sb.where.groupId = `has(groups, ${sqlstring.escape(groupId)})` but getEventsCount doesn't, so counts ignore the group filter; update the count path in getEventsCount (and the alternate count logic around the same area referenced 681-709) to carry the same groupId handling by adding the same sb.where.groupId assignment (or equivalent condition) when groupId is present so the count reflects the filtered result.packages/db/src/services/group.service.ts-110-141 (1)
110-141:⚠️ Potential issue | 🟠 MajorMultiply numeric cursors by
takebefore using them asOFFSET.Elsewhere in the repo the numeric cursor is treated as a page index. Here page 2 starts at row 1 instead of row
take, so both list endpoints will overlap heavily.🛠️ Suggested fix
const rows = await chQuery<IClickhouseGroup>(` SELECT project_id, id, type, name, properties, created_at, version FROM ${TABLE_NAMES.groups} FINAL WHERE ${conditions.join(' AND ')} ORDER BY created_at DESC LIMIT ${take} - OFFSET ${cursor ?? 0} + OFFSET ${Math.max(0, (cursor ?? 0) * take)} `);- const offset = Math.max(0, cursor ?? 0); + const offset = Math.max(0, (cursor ?? 0) * take);Also applies to: 304-344
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/src/services/group.service.ts` around lines 110 - 141, The OFFSET calculation in getGroupList uses the numeric cursor as a raw row offset instead of a page index; update the OFFSET expression to multiply cursor by take (use (cursor ?? 0) * take) so the cursor parameter behaves as a page index, and apply the same change to the other list function that uses cursor/take (the similar block around lines 304-344) to avoid overlapping pages; ensure you only adjust the OFFSET logic and preserve existing defaulting when cursor is undefined.packages/db/src/services/chart.service.ts-164-179 (1)
164-179:⚠️ Potential issue | 🟠 MajorJoin
_gwheneverevent.propertytargetsgroup.*.
needsGroupArrayJoinonly looks at filters, breakdowns, andevent.segment. Aproperty_sum/property_average/etc. overgroup.*will still emit_g...expressions without the_gjoin, and the time-series path also resolves the property withoutprojectId. That breaks group-metric charts unless some separate group filter/breakdown happens to be present.🛠️ Suggested fix
- const needsGroupArrayJoin = - anyFilterOnGroup || anyBreakdownOnGroup || event.segment === 'group'; + const anyMetricOnGroup = event.property?.startsWith('group.') ?? false; + const needsGroupArrayJoin = + anyFilterOnGroup || + anyBreakdownOnGroup || + event.segment === 'group' || + anyMetricOnGroup; ... - const propertyKey = getSelectPropertyKey(event.property); + const propertyKey = getSelectPropertyKey(event.property, projectId);Apply the same
anyMetricOnGroupguard ingetAggregateChartSql()as well.Also applies to: 344-345, 456-466, 598-599
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/src/services/chart.service.ts` around lines 164 - 179, The code only checks filters, breakdowns and event.segment to decide adding the _g join, but metrics can reference group.* too; in getAggregateChartSql() add a check (e.g., anyMetricOnGroup) that scans event.metrics (or the metric parsing path) for metric.property names starting with 'group.' and include that in the needsGroupArrayJoin boolean (so needsGroupArrayJoin = anyFilterOnGroup || anyBreakdownOnGroup || anyMetricOnGroup || event.segment === 'group'), then ensure the existing addCte('_g', ...), sb.joins.groups and sb.joins.groups_table logic runs when this expanded guard is true; apply the same change in the other affected spots noted (around the other ranges ~344-345, 456-466, 598-599) so any metric that targets group.* triggers the _g join.packages/db/src/services/event.service.ts-647-652 (1)
647-652:⚠️ Potential issue | 🟠 MajorDe-duplicate events after the group
ARRAY JOIN.These joins expand one event into one row per matching group. In the list/count paths that means duplicate events, inflated totals, and unstable pagination whenever an event belongs to two matching groups. Filter via a semi-join/
arrayExists, or wrap the joined query in an outerSELECT DISTINCT/countDistinct(id)before paginating.Also applies to: 721-726, 1098-1105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/src/services/event.service.ts` around lines 647 - 652, The current ARRAY JOIN expansion via sb.joins.groups / sb.joins.groups_cte causes one event to become multiple rows when it matches multiple groups; replace this expansion with a de-duplicating approach: instead of setting sb.joins.groups = 'ARRAY JOIN groups AS _group_id', change the filter generation to use a semi-join or arrayExists predicate (e.g. arrayExists(g -> g.id = _desired, groups) or an EXISTS-style join against TABLE_NAMES.groups) so events are only matched once, and for count paths use countDistinct(id) (or wrap the joined result in an outer SELECT DISTINCT id) before applying pagination; apply the same change where sb.joins.groups and sb.joins.groups_cte are set elsewhere (the other occurrences of the ARRAY JOIN logic) and ensure groupFilters (the filters derived from f.name.startsWith('group.')) are translated to the new arrayExists/semi-join logic.packages/db/src/services/chart.service.ts-702-705 (1)
702-705:⚠️ Potential issue | 🟠 MajorUse
ANDfor multi-valuedoesNotContaingroup filters.With
OR, a group that contains one forbidden term still matches as soon as it misses another one. Negative predicates need to be combined withAND.🛠️ Suggested fix
case 'doesNotContain': { where[id] = - `(${value.map((val) => `${whereFrom} NOT LIKE ${sqlstring.escape(`%${String(val).trim()}%`)}`).join(' OR ')})`; + `(${value.map((val) => `${whereFrom} NOT LIKE ${sqlstring.escape(`%${String(val).trim()}%`)}`).join(' AND ')})`; break; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/src/services/chart.service.ts` around lines 702 - 705, The filter for the 'doesNotContain' case in chart.service.ts builds a multi-value negative predicate using OR, which is incorrect; change the join to use AND so every forbidden term is required to not match. Locate the case 'doesNotContain' where it assigns to where[id] (uses whereFrom and sqlstring.escape over value.map(...)), and update the join from ' OR ' to ' AND ' so the resulting clause is (... NOT LIKE '%val1%' AND ... NOT LIKE '%valN%').packages/db/src/services/group.service.ts-100-359 (1)
100-359: 🛠️ Refactor suggestion | 🟠 MajorPlease start the new group service on
clix/query-functions`.This file hand-assembles every ClickHouse
SELECTwith template strings. Since this is a brand-new query surface, it is the right place to align with the shared builder before more pagination/filter logic diverges.As per coding guidelines
**/*.{ts,tsx,js,jsx}: When writing ClickHouse queries, always use the custom query builder from./packages/db/src/clickhouse/query-builder.tsand./packages/db/src/clickhouse/query-functions.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/src/services/group.service.ts` around lines 100 - 359, The file builds raw ClickHouse SELECTs with template strings; replace those with the shared query builder/query-functions API (import from clickhouse/query-builder.ts and clickhouse/query-functions.ts) for all functions that run SELECTs — e.g., getGroupById, getGroupList, getGroupListCount, getGroupTypes, getGroupPropertyKeys, getGroupStats, getGroupsByIds, and getGroupMemberProfiles — by converting WHERE/ORDER/LIMIT/OFFSET/ARRAY JOIN/COUNT logic into the builder calls and query-functions helpers, use the builder to safely compose escaped conditions (project_id, deleted, type, search, id IN, has(groups,...), etc.), and then pass the built query into chQuery; ensure the same projections (project_id, id, type, name, properties, created_at, version) and the count/arrayJoin expressions are preserved when translating into the builder API.apps/start/src/modals/edit-group.tsx-84-104 (1)
84-104:⚠️ Potential issue | 🟠 MajorAdd accessible names to the property-row controls.
The two raw inputs and the icon-only remove button have no accessible label, so screen-reader users cannot tell which field is the key, which is the value, or what the trash button does.
♿ Suggested fix
{fields.map((field, index) => ( <div key={field.id} className="flex gap-2"> <input + aria-label={`Property key ${index + 1}`} className="h-9 flex-1 rounded-md border bg-background px-3 text-sm placeholder:text-muted-foreground focus:outline-none focus:ring-1 focus:ring-ring" placeholder="key" {...register(`properties.${index}.key`)} /> <input + aria-label={`Property value ${index + 1}`} className="h-9 flex-1 rounded-md border bg-background px-3 text-sm placeholder:text-muted-foreground focus:outline-none focus:ring-1 focus:ring-ring" placeholder="value" {...register(`properties.${index}.value`)} /> <Button type="button" variant="ghost" size="icon" className="shrink-0" + aria-label={`Remove property ${index + 1}`} onClick={() => remove(index)} > <Trash2Icon className="size-4" /> </Button> </div> ))}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/start/src/modals/edit-group.tsx` around lines 84 - 104, The property row inputs and icon-only remove Button rendered inside fields.map lack accessible names; update the JSX for the key/value inputs (registered via register(`properties.${index}.key` / `.value`)) to include explicit labels or aria attributes—e.g., generate unique ids like `properties-${index}-key` and `properties-${index}-value`, add <label htmlFor="..."> (visually hidden if needed) or aria-label/aria-labelledby on the inputs, and give the Trash2Icon Button a clear accessible name (aria-label="Remove property" or aria-labelledby referencing the row label). Ensure ids incorporate index so register and remove(index) still align with the inputs.packages/db/src/services/group.service.ts-188-209 (1)
188-209:⚠️ Potential issue | 🟠 MajorMerge and flatten updated properties before writing.
updateGroup()currently replaces the whole property map whendata.propertiesis present, and it skips thetoDots()normalization used inupsertGroup(). Partial updates will silently drop existing keys.🛠️ Suggested fix
const updated = { id, projectId, type: data.type ?? existing.type, name: data.name ?? existing.name, - properties: (data.properties ?? existing.properties) as Record< - string, - string - >, + properties: toDots({ + ...(existing.properties ?? {}), + ...(data.properties ?? {}), + }) as Record<string, string>, createdAt: existing.createdAt, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/src/services/group.service.ts` around lines 188 - 209, updateGroup currently overwrites properties when data.properties is provided and skips the toDots() normalization used by upsertGroup; change updateGroup (and the object assigned to `updated`) to merge existing.properties with data.properties (e.g., merged = { ...existing.properties, ...data.properties }) then pass the merged result through the same toDots(...) normalization before casting to Record<string, string>, and then call writeGroupToCh(updated) and return the merged/normalized properties; reference functions/idents: updateGroup, getGroupById, upsertGroup, toDots, writeGroupToCh.apps/start/src/modals/add-group.tsx-82-102 (1)
82-102:⚠️ Potential issue | 🟠 MajorLabel the dynamic property controls.
These key/value inputs rely on placeholder text, and the icon-only remove button has no accessible name. That makes the properties editor hard to use with screen readers. Add visible labels or
aria-labels for the fields and the remove action.♿ Suggested fix
{fields.map((field, index) => ( <div key={field.id} className="flex gap-2"> <input + aria-label={`Property key ${index + 1}`} className="h-9 flex-1 rounded-md border bg-background px-3 text-sm placeholder:text-muted-foreground focus:outline-none focus:ring-1 focus:ring-ring" placeholder="key" {...register(`properties.${index}.key`)} /> <input + aria-label={`Property value ${index + 1}`} className="h-9 flex-1 rounded-md border bg-background px-3 text-sm placeholder:text-muted-foreground focus:outline-none focus:ring-1 focus:ring-ring" placeholder="value" {...register(`properties.${index}.value`)} /> <Button + aria-label={`Remove property ${index + 1}`} type="button" variant="ghost" size="icon" className="shrink-0" onClick={() => remove(index)} > <Trash2Icon className="size-4" /> </Button> </div> ))}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/start/src/modals/add-group.tsx` around lines 82 - 102, The key/value inputs and remove Button rendered in the fields.map block lack accessible names; update the JSX around fields.map (the inputs using register(`properties.${index}.key`) and register(`properties.${index}.value`) and the remove Button that calls remove(index) / renders Trash2Icon) to provide accessible labels—either add visible <label> elements associated with each input or add descriptive aria-label attributes like `aria-label={`Property key ${index + 1}`}` and `aria-label={`Property value ${index + 1}`}`, and give the remove Button an aria-label such as `aria-label={`Remove property ${index + 1}`}` so screen readers can identify each control.apps/start/src/modals/index.tsx-39-40 (1)
39-40:⚠️ Potential issue | 🟠 MajorVerify that
EditGroupprops are safe to send to analytics.EditGroup is opened with the full group object including its
propertiesfield (line 111 in groups tab), which contains user-defined key-value pairs. These props flow through the wildcardonPushModal('*', ...)handler (lines 86–90) that forwards all modal props toop.screenView()for analytics. This means arbitrary user-defined group metadata will be emitted to analytics.Either exclude
EditGroupfrom the wildcard analytics hook, or filter the props passed to it to remove sensitive fields likeproperties.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/start/src/modals/index.tsx` around lines 39 - 40, The wildcard analytics handler onPushModal('*', ...) is sending all modal props (including user-defined group.properties from EditGroup) to op.screenView(); update the handler so it does not forward sensitive fields: either explicitly skip analytics for modalName === 'EditGroup' or sanitize the props before calling op.screenView() by removing the properties field (and any other sensitive keys) from the payload; locate the onPushModal('*', ...) handler and implement the conditional exclusion or shallow-clone-and-delete behavior for EditGroup props before calling op.screenView().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 975c7044-6654-4b3d-9830-5f6135d31a7e
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (84)
apps/api/scripts/test.tsapps/api/src/controllers/track.controller.tsapps/public/content/docs/dashboard/understand-the-overview.mdxapps/start/src/components/events/table/columns.tsxapps/start/src/components/groups/group-member-growth.tsxapps/start/src/components/groups/table/columns.tsxapps/start/src/components/groups/table/index.tsxapps/start/src/components/profiles/profile-groups.tsxapps/start/src/components/profiles/table/columns.tsxapps/start/src/components/report-chart/funnel/chart.tsxapps/start/src/components/report/ReportSegment.tsxapps/start/src/components/report/sidebar/PropertiesCombobox.tsxapps/start/src/components/sessions/table/columns.tsxapps/start/src/components/sidebar-project-menu.tsxapps/start/src/components/ui/data-table/data-table-toolbar.tsxapps/start/src/components/ui/select.tsxapps/start/src/modals/add-group.tsxapps/start/src/modals/edit-group.tsxapps/start/src/modals/index.tsxapps/start/src/modals/view-chart-users.tsxapps/start/src/routeTree.gen.tsapps/start/src/routes/_app.$organizationId.$projectId.groups.tsxapps/start/src/routes/_app.$organizationId.$projectId.groups_.$groupId._tabs.events.tsxapps/start/src/routes/_app.$organizationId.$projectId.groups_.$groupId._tabs.index.tsxapps/start/src/routes/_app.$organizationId.$projectId.groups_.$groupId._tabs.members.tsxapps/start/src/routes/_app.$organizationId.$projectId.groups_.$groupId._tabs.tsxapps/start/src/routes/_app.$organizationId.$projectId.profiles.$profileId._tabs.index.tsxapps/start/src/routes/_app.$organizationId.$projectId.sessions_.$sessionId.tsxapps/start/src/utils/title.tsapps/testbed/.gitignoreapps/testbed/index.htmlapps/testbed/package.jsonapps/testbed/scripts/copy-op1.mjsapps/testbed/src/App.tsxapps/testbed/src/analytics.tsapps/testbed/src/main.tsxapps/testbed/src/pages/Cart.tsxapps/testbed/src/pages/Checkout.tsxapps/testbed/src/pages/Login.tsxapps/testbed/src/pages/Product.tsxapps/testbed/src/pages/Shop.tsxapps/testbed/src/styles.cssapps/testbed/src/types.tsapps/testbed/tsconfig.jsonapps/testbed/vite.config.tsapps/worker/src/boot-cron.tsapps/worker/src/jobs/cron.tsapps/worker/src/jobs/events.incoming-event.tsapps/worker/src/jobs/events.incoming-events.test.tsdocker-compose.ymlpackages/common/scripts/get-referrers.tspackages/constants/index.tspackages/db/code-migrations/11-add-groups.tspackages/db/index.tspackages/db/prisma/schema.prismapackages/db/src/buffers/event-buffer.tspackages/db/src/buffers/group-buffer.tspackages/db/src/buffers/index.tspackages/db/src/buffers/profile-buffer.test.tspackages/db/src/buffers/profile-buffer.tspackages/db/src/buffers/session-buffer.tspackages/db/src/clickhouse/client.tspackages/db/src/clickhouse/query-builder.tspackages/db/src/engine/index.tspackages/db/src/services/chart.service.tspackages/db/src/services/conversion.service.tspackages/db/src/services/event.service.tspackages/db/src/services/funnel.service.tspackages/db/src/services/group.service.tspackages/db/src/services/import.service.tspackages/db/src/services/profile.service.tspackages/db/src/services/session.service.tspackages/importer/src/providers/mixpanel.test.tspackages/importer/src/providers/mixpanel.tspackages/importer/src/providers/umami.tspackages/logger/index.tspackages/queue/src/queues.tspackages/sdks/sdk/src/index.tspackages/trpc/src/root.tspackages/trpc/src/routers/chart.tspackages/trpc/src/routers/event.tspackages/trpc/src/routers/group.tspackages/validation/src/track.validation.tsscripts/seed-events.mjs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/db/src/services/chart.service.ts (1)
125-133:⚠️ Potential issue | 🟠 MajorFunction signature silently ignores
limit,chartType,metric,previous, andoffsetparameters.The destructuring omits several fields from
IGetChartDataInputthat callers are actively passing (seepackages/db/src/engine/fetch.tslines 51-55). These parameters are silently discarded without TypeScript warnings due to object spread semantics.Specifically:
limit(line 54 in fetch.ts) — pagination is completely bypassedchartType(line 51) — chart-type-specific rendering logic is lostmetric,previous,offset— also passed but ignoredCompare with
getAggregateChartSql(line 426), which explicitly declares unused parameters viaOmit<IGetChartDataInput, 'interval' | 'chartType'>and actually applieslimitat lines 639-640. Use the same pattern forgetChartSql: either explicitly destructure and use these fields, or update the type signature to reflect what the function actually consumes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/src/services/chart.service.ts` around lines 125 - 133, getChartSql currently destructures IGetChartDataInput but omits limit, chartType, metric, previous, and offset so callers' values are silently ignored; update the getChartSql signature to either (a) accept the same typed shape as getAggregateChartSql (e.g., Omit<IGetChartDataInput, 'interval' | 'chartType'> or the full IGetChartDataInput & { timezone: string }) and explicitly destructure limit, chartType, metric, previous, and offset, or (b) explicitly add those parameters to the signature and then apply them (for example, apply limit/offset to the SQL pagination and use chartType/metric/previous where chart-specific logic is needed) so values passed from fetch.ts are not dropped; reference getChartSql, IGetChartDataInput, and the pattern used in getAggregateChartSql to mirror how limit is applied.
🧹 Nitpick comments (1)
packages/db/src/services/chart.service.ts (1)
681-740: Missing numeric comparison operators (gt,lt,gte,lte) for group filters.The
group.*filter handling supports string operators but lacks numeric comparisons (gt,lt,gte,lte) that exist forproperties.*filters (lines 882-953).If group properties can be numeric (e.g.,
group.properties.member_count), these operators would silently fall through and not generate any WHERE clause.Please verify if this is intentional or if numeric operators should be added for completeness.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/controllers/track.controller.ts`:
- Around line 334-356: The code currently falls back to context.deviceId when
determining profileId and always upserts a profile whenever profileId exists;
change this so upsertProfile is called only when the request explicitly provided
payload.profileId. Locate the variables and calls: the destructuring "const {
id, type, name, properties = {} } = payload", the "const profileId =
payload.profileId ?? context.deviceId", the group creation
"groupBuffer.add(...)" and the upsert call "upsertProfile({...})"; keep
groupBuffer.add as-is but replace the conditional "if (profileId) { ...
upsertProfile(...)}" with a check that payload.profileId is non-null/defined
(e.g., if (payload.profileId != null) { upsertProfile({... id:
String(payload.profileId), isExternal: true, groups: [id], projectId:
context.projectId }) }), ensuring isExternal and id derive from
payload.profileId rather than the fallback context.deviceId.
In `@packages/db/src/services/chart.service.ts`:
- Around line 655-658: The getEventFiltersWhereClause function currently skips
filters whose name startsWith('group.') when projectId is undefined, causing
silent no-op filtering; update getEventFiltersWhereClause to detect any filters
with name starting with 'group.' and, if projectId is not provided, throw a
clear error (or return an explicit failure) instead of silently skipping them so
callers (e.g., session.service.ts) must supply projectId or intentionally avoid
group.* filters; specifically add a defensive check before the existing group.*
handling that inspects filters for /^group\./ and raises an informative
exception referencing getEventFiltersWhereClause and the missing projectId.
- Around line 97-105: getSelectPropertyKey must defensively handle group.*
properties when projectId is undefined: add an early branch in
getSelectPropertyKey that checks if property.startsWith('group.') and projectId
is falsy, and return a safe SQL placeholder (e.g., an empty string literal)
instead of falling through to the invalid column name; this mirrors the
defensive check pattern used in getEventFiltersWhereClause and prevents
generating "group.xyz" columns that don't exist—refer to getSelectPropertyKey
and getGroupPropertySql to locate where to add the check.
In `@packages/db/src/services/event.service.ts`:
- Line 185: transformEvent() (and the similar occurrences around the other
mentioned block) must not collapse an absent/groups-not-selected column into an
empty array; instead keep it undefined when the DB projection didn't include it.
Change the groups assignment that currently reads like "groups: event.groups ??
[]" to only set groups when event.groups is defined (e.g. groups: event.groups
!== undefined ? event.groups : undefined), and apply the same pattern to the
other two spots referenced so callers that don't project groups (such as
getEventList()/the TRPC events route) receive undefined rather than an empty
array.
- Around line 647-652: The ARRAY JOIN in the groups join (sb.joins.groups and
sb.joins.groups_cte) duplicates events when an event has multiple group rows,
causing getEventList() pagination duplicates and getEventsCount() overcounting;
change the join strategy to a non-duplicating existence/semi-join (e.g., replace
the ARRAY JOIN pattern with a WHERE EXISTS or LEFT ANY JOIN on a
grouped/subselected set of group ids) or ensure results are re-aggregated by
event id before returning/counting so that each event appears only once; update
the code paths used by getEventList() and getEventsCount() to use this new
non-duplicating join (reference sb.joins.groups, sb.joins.groups_cte,
TABLE_NAMES.groups, projectId, and filters).
- Around line 1099-1110: The group-filter JOIN/ARRAY JOIN logic duplicated via
q.rawJoin/q.rawJoin(...) in getEventList/getEventsCount should be extracted into
a shared ClickHouse query-function (e.g., export a function like
applyGroupFilterJoins or buildGroupFilterJoins in clickhouse/query-functions.ts)
and invoked from event.service.ts instead of inlining the SQL; update the code
that currently calls q.rawJoin('ARRAY JOIN groups AS _group_id') and the LEFT
ANY JOIN using TABLE_NAMES.groups and _group_id to call that new helper, and
ensure getEventFiltersWhereClause usage remains for the WHERE clause so all
three places reuse the same helper.
In `@packages/db/src/services/group.service.ts`:
- Around line 328-348: The current member query only reads membership from
events (SELECT ... FROM ${TABLE_NAMES.events}) but must also include profiles
with group membership stored in profiles.groups (set via upsertProfile in
handleGroup); update the query used in group.service.ts (the chQuery block) to
union or join profiles where profiles.groups contains the groupId (e.g., include
profile IDs from ${TABLE_NAMES.profiles} where has(groups,
${sqlstring.escape(groupId)})), ensure duplicates are deduplicated (profile_id),
preserve or compute a sensible last_seen (use max(created_at) from events when
available, nullable otherwise), and keep the existing ORDER BY/ LIMIT/ OFFSET
and the filtering via searchCondition so profiles added via upsertProfile are
returned as members even if they have no grouped events.
- Around line 100-106: The query in group.service.ts currently builds a raw SQL
string with template literals and sqlstring.escape for chQuery (selecting
project_id, id, type, name, properties, created_at, version from
TABLE_NAMES.groups FINAL ...), which violates the repo rule to use the
ClickHouse query builder; refactor each raw query (including the one using
chQuery<IClickhouseGroup> and the other occurrences called out) to use the
shared builder and helper functions from clickhouse/query-builder.ts and
clickhouse/query-functions.ts—replace string interpolation of projectId and id
with parameterized builder calls, construct the SELECT and WHERE clauses via the
query-builder API, and invoke chQuery with the built query object so escaping
and composition follow the established helpers.
- Around line 212-213: The returned object merges existing and updated so it
preserves the old updatedAt; after calling writeGroupToCh(updated) you should
return the actual persisted row or at least update updatedAt from the persisted
version—so re-read the group (e.g., fetch by id using the existing read method)
after writeGroupToCh(updated) and return that fresh row, or obtain the new
version/timestamp from writeGroupToCh and set updatedAt on the returned object
instead of merging existing.updatedAt.
In `@packages/validation/src/track.validation.ts`:
- Line 10: The schema for profileId currently allows both strings and numbers
which lets numeric 0 pass validation but later be treated as missing; update the
profileId definition in track.validation.ts to normalize to a single output type
(preferably string) by replacing the union with a string-only schema or by using
a preprocessing step (e.g., z.preprocess) to coerce numbers to strings so that
the exported/parsed value for profileId is always a non-empty string for
downstream code (refer to the profileId schema and the surrounding track
validation exports).
---
Outside diff comments:
In `@packages/db/src/services/chart.service.ts`:
- Around line 125-133: getChartSql currently destructures IGetChartDataInput but
omits limit, chartType, metric, previous, and offset so callers' values are
silently ignored; update the getChartSql signature to either (a) accept the same
typed shape as getAggregateChartSql (e.g., Omit<IGetChartDataInput, 'interval' |
'chartType'> or the full IGetChartDataInput & { timezone: string }) and
explicitly destructure limit, chartType, metric, previous, and offset, or (b)
explicitly add those parameters to the signature and then apply them (for
example, apply limit/offset to the SQL pagination and use
chartType/metric/previous where chart-specific logic is needed) so values passed
from fetch.ts are not dropped; reference getChartSql, IGetChartDataInput, and
the pattern used in getAggregateChartSql to mirror how limit is applied.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2ad29b1a-3a08-4b92-b48c-0207ef0b3dc8
📒 Files selected for processing (7)
apps/api/src/controllers/track.controller.tspackages/common/scripts/get-referrers.tspackages/db/src/services/chart.service.tspackages/db/src/services/event.service.tspackages/db/src/services/group.service.tspackages/sdks/sdk/src/index.tspackages/validation/src/track.validation.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/common/scripts/get-referrers.ts
- packages/sdks/sdk/src/index.ts
Comment on lines
+334
to
+356
| const { id, type, name, properties = {} } = payload; | ||
| const profileId = payload.profileId ?? context.deviceId; | ||
|
|
||
| const promises: Promise<unknown>[] = []; | ||
| promises.push( | ||
| groupBuffer.add({ | ||
| id, | ||
| projectId: context.projectId, | ||
| type, | ||
| name, | ||
| properties, | ||
| }) | ||
| ); | ||
|
|
||
| if (profileId) { | ||
| promises.push( | ||
| upsertProfile({ | ||
| id: String(profileId), | ||
| projectId: context.projectId, | ||
| isExternal: !!(payload.profileId ?? context.identity?.profileId), | ||
| groups: [id], | ||
| }) | ||
| ); |
Contributor
There was a problem hiding this comment.
Don’t attach group calls to context.deviceId when profileId is omitted.
profileId is optional in validation, so a request without it should just create/update the group. Falling back to context.deviceId here means every group call creates membership on a device-scoped profile, which is especially risky for server-side callers where many requests can share the same derived device id. Only upsert a profile when the request explicitly supplied payload.profileId.
Proposed fix
- const profileId = payload.profileId ?? context.deviceId;
+ const profileId = payload.profileId;
@@
- if (profileId) {
+ if (profileId !== undefined) {
promises.push(
upsertProfile({
id: String(profileId),
projectId: context.projectId,
- isExternal: !!(payload.profileId ?? context.identity?.profileId),
+ isExternal: true,
groups: [id],
})
);
}📝 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 { id, type, name, properties = {} } = payload; | |
| const profileId = payload.profileId ?? context.deviceId; | |
| const promises: Promise<unknown>[] = []; | |
| promises.push( | |
| groupBuffer.add({ | |
| id, | |
| projectId: context.projectId, | |
| type, | |
| name, | |
| properties, | |
| }) | |
| ); | |
| if (profileId) { | |
| promises.push( | |
| upsertProfile({ | |
| id: String(profileId), | |
| projectId: context.projectId, | |
| isExternal: !!(payload.profileId ?? context.identity?.profileId), | |
| groups: [id], | |
| }) | |
| ); | |
| const { id, type, name, properties = {} } = payload; | |
| const profileId = payload.profileId; | |
| const promises: Promise<unknown>[] = []; | |
| promises.push( | |
| groupBuffer.add({ | |
| id, | |
| projectId: context.projectId, | |
| type, | |
| name, | |
| properties, | |
| }) | |
| ); | |
| if (profileId !== undefined) { | |
| promises.push( | |
| upsertProfile({ | |
| id: String(profileId), | |
| projectId: context.projectId, | |
| isExternal: true, | |
| groups: [id], | |
| }) | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/controllers/track.controller.ts` around lines 334 - 356, The
code currently falls back to context.deviceId when determining profileId and
always upserts a profile whenever profileId exists; change this so upsertProfile
is called only when the request explicitly provided payload.profileId. Locate
the variables and calls: the destructuring "const { id, type, name, properties =
{} } = payload", the "const profileId = payload.profileId ?? context.deviceId",
the group creation "groupBuffer.add(...)" and the upsert call
"upsertProfile({...})"; keep groupBuffer.add as-is but replace the conditional
"if (profileId) { ... upsertProfile(...)}" with a check that payload.profileId
is non-null/defined (e.g., if (payload.profileId != null) { upsertProfile({...
id: String(payload.profileId), isExternal: true, groups: [id], projectId:
context.projectId }) }), ensuring isExternal and id derive from
payload.profileId rather than the fallback context.deviceId.
Comment on lines
+97
to
+105
| export function getSelectPropertyKey(property: string, projectId?: string) { | ||
| if (property === 'has_profile') { | ||
| return `if(profile_id != device_id, 'true', 'false')`; | ||
| } | ||
|
|
||
| // Handle group properties — requires ARRAY JOIN + _g JOIN to be present in query | ||
| if (property.startsWith('group.') && projectId) { | ||
| return getGroupPropertySql(property); | ||
| } |
Contributor
There was a problem hiding this comment.
group.* properties without projectId generate invalid SQL.
When projectId is undefined, group.* properties bypass the routing on line 103-105 and fall through to line 112-114, returning the property name as-is (e.g., "group.name"). This column doesn't exist in the events table, causing a ClickHouse query failure.
This is consistent with the issue in getEventFiltersWhereClause. Consider adding a similar defensive check:
🛡️ Proposed fix
// Handle group properties — requires ARRAY JOIN + _g JOIN to be present in query
- if (property.startsWith('group.') && projectId) {
+ if (property.startsWith('group.')) {
+ if (!projectId) {
+ throw new Error(`projectId is required for group property: ${property}`);
+ }
return getGroupPropertySql(property);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/db/src/services/chart.service.ts` around lines 97 - 105,
getSelectPropertyKey must defensively handle group.* properties when projectId
is undefined: add an early branch in getSelectPropertyKey that checks if
property.startsWith('group.') and projectId is falsy, and return a safe SQL
placeholder (e.g., an empty string literal) instead of falling through to the
invalid column name; this mirrors the defensive check pattern used in
getEventFiltersWhereClause and prevents generating "group.xyz" columns that
don't exist—refer to getSelectPropertyKey and getGroupPropertySql to locate
where to add the check.
Comment on lines
+655
to
+658
| export function getEventFiltersWhereClause( | ||
| filters: IChartEventFilter[], | ||
| projectId?: string | ||
| ) { |
Contributor
There was a problem hiding this comment.
Silent failure when projectId is not provided for group.* filters.
The projectId parameter is optional, but group.* filters require it (line 682: if (name.startsWith('group.') && projectId)). When callers omit projectId:
group.*filters are silently skipped — no WHERE clause is generated- Query returns unfiltered results, potentially a correctness/data-leak issue
Existing callers like session.service.ts (line 193) call this without projectId. If those ever receive group.* filters, they'll silently fail.
Consider either:
- Making
projectIdrequired and updating all callers - Throwing an error when
group.*filters are present butprojectIdis missing - Documenting this limitation clearly in JSDoc
🛡️ Proposed defensive check
+ // Handle group. prefixed filters (requires ARRAY JOIN + _g JOIN in query)
+ if (name.startsWith('group.')) {
+ if (!projectId) {
+ throw new Error(`projectId is required for group.* filter: ${name}`);
+ }
const whereFrom = getGroupPropertySql(name);Also applies to: 681-682
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/db/src/services/chart.service.ts` around lines 655 - 658, The
getEventFiltersWhereClause function currently skips filters whose name
startsWith('group.') when projectId is undefined, causing silent no-op
filtering; update getEventFiltersWhereClause to detect any filters with name
starting with 'group.' and, if projectId is not provided, throw a clear error
(or return an explicit failure) instead of silently skipping them so callers
(e.g., session.service.ts) must supply projectId or intentionally avoid group.*
filters; specifically add a defensive check before the existing group.* handling
that inspects filters for /^group\./ and raises an informative exception
referencing getEventFiltersWhereClause and the missing projectId.
| sdkVersion: event.sdk_version, | ||
| profile: event.profile, | ||
| revenue: event.revenue, | ||
| groups: event.groups ?? [], |
Contributor
There was a problem hiding this comment.
Don't collapse “not selected” into an empty groups array.
getEventList() only projects groups when select.groups is enabled, but transformEvent() now normalizes missing data to []. The TRPC events route in packages/trpc/src/routers/event.ts:119-179 does not request that column, so grouped events will be returned as groups: [] instead of their real memberships. Either project groups on that path by default or keep it undefined when the column was not selected.
Also applies to: 605-607
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/db/src/services/event.service.ts` at line 185, transformEvent() (and
the similar occurrences around the other mentioned block) must not collapse an
absent/groups-not-selected column into an empty array; instead keep it undefined
when the DB projection didn't include it. Change the groups assignment that
currently reads like "groups: event.groups ?? []" to only set groups when
event.groups is defined (e.g. groups: event.groups !== undefined ? event.groups
: undefined), and apply the same pattern to the other two spots referenced so
callers that don't project groups (such as getEventList()/the TRPC events route)
receive undefined rather than an empty array.
Comment on lines
+647
to
+652
| // Join groups table if any filter uses group fields | ||
| const groupFilters = filters.filter((f) => f.name.startsWith('group.')); | ||
| if (groupFilters.length > 0) { | ||
| sb.joins.groups = 'ARRAY JOIN groups AS _group_id'; | ||
| sb.joins.groups_cte = `LEFT ANY JOIN (SELECT id, name, type, properties FROM ${TABLE_NAMES.groups} FINAL WHERE project_id = ${sqlstring.escape(projectId)}) AS _g ON _g.id = _group_id`; | ||
| } |
Contributor
There was a problem hiding this comment.
ARRAY JOIN here will duplicate matching events.
This turns one event into one row per group. If an event matches through multiple group rows, getEventList() will paginate duplicates and the same pattern in getEventsCount() will overcount. Please filter groups through an existence/semi-join pattern, or re-aggregate on event id before returning/counting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/db/src/services/event.service.ts` around lines 647 - 652, The ARRAY
JOIN in the groups join (sb.joins.groups and sb.joins.groups_cte) duplicates
events when an event has multiple group rows, causing getEventList() pagination
duplicates and getEventsCount() overcounting; change the join strategy to a
non-duplicating existence/semi-join (e.g., replace the ARRAY JOIN pattern with a
WHERE EXISTS or LEFT ANY JOIN on a grouped/subselected set of group ids) or
ensure results are re-aggregated by event id before returning/counting so that
each event appears only once; update the code paths used by getEventList() and
getEventsCount() to use this new non-duplicating join (reference
sb.joins.groups, sb.joins.groups_cte, TABLE_NAMES.groups, projectId, and
filters).
Comment on lines
+1099
to
+1110
| Object.values( | ||
| getEventFiltersWhereClause(filters, projectId) | ||
| ).join(' AND ') | ||
| ); | ||
| const groupFilters = filters.filter((f) => | ||
| f.name.startsWith('group.') | ||
| ); | ||
| if (groupFilters.length > 0) { | ||
| q.rawJoin('ARRAY JOIN groups AS _group_id'); | ||
| q.rawJoin( | ||
| `LEFT ANY JOIN (SELECT id, name, type, properties FROM ${TABLE_NAMES.groups} FINAL WHERE project_id = ${sqlstring.escape(projectId)}) AS _g ON _g.id = _group_id` | ||
| ); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Keep the new group filter path in the shared ClickHouse builder.
This adds a third copy of the group-filter SQL via rawWhere/rawJoin, which makes the event query paths easy to drift apart. Please extract this into the shared clix/query-functions path and reuse it here, in getEventList(), and in getEventsCount(). As per coding guidelines, "When writing ClickHouse queries, always use the custom query builder from ./packages/db/src/clickhouse/query-builder.ts and ./packages/db/src/clickhouse/query-functions.ts"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/db/src/services/event.service.ts` around lines 1099 - 1110, The
group-filter JOIN/ARRAY JOIN logic duplicated via q.rawJoin/q.rawJoin(...) in
getEventList/getEventsCount should be extracted into a shared ClickHouse
query-function (e.g., export a function like applyGroupFilterJoins or
buildGroupFilterJoins in clickhouse/query-functions.ts) and invoked from
event.service.ts instead of inlining the SQL; update the code that currently
calls q.rawJoin('ARRAY JOIN groups AS _group_id') and the LEFT ANY JOIN using
TABLE_NAMES.groups and _group_id to call that new helper, and ensure
getEventFiltersWhereClause usage remains for the WHERE clause so all three
places reuse the same helper.
Comment on lines
+100
to
+106
| const rows = await chQuery<IClickhouseGroup>(` | ||
| SELECT project_id, id, type, name, properties, created_at, version | ||
| FROM ${TABLE_NAMES.groups} FINAL | ||
| WHERE project_id = ${sqlstring.escape(projectId)} | ||
| AND id = ${sqlstring.escape(id)} | ||
| AND deleted = 0 | ||
| `); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use the ClickHouse query builder here instead of raw SQL strings.
This file hand-builds ClickHouse queries with template literals and sqlstring.escape(). The repo rule for TS/JS query code is to use the shared query builder/functions, so this should be refactored before the query surface grows further.
As per coding guidelines, **/*.{ts,tsx,js,jsx}: When writing ClickHouse queries, always use the custom query builder from ./packages/db/src/clickhouse/query-builder.ts and ./packages/db/src/clickhouse/query-functions.ts.
Also applies to: 134-141, 165-169, 174-179, 238-243, 266-276, 298-304, 328-348
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/db/src/services/group.service.ts` around lines 100 - 106, The query
in group.service.ts currently builds a raw SQL string with template literals and
sqlstring.escape for chQuery (selecting project_id, id, type, name, properties,
created_at, version from TABLE_NAMES.groups FINAL ...), which violates the repo
rule to use the ClickHouse query builder; refactor each raw query (including the
one using chQuery<IClickhouseGroup> and the other occurrences called out) to use
the shared builder and helper functions from clickhouse/query-builder.ts and
clickhouse/query-functions.ts—replace string interpolation of projectId and id
with parameterized builder calls, construct the SELECT and WHERE clauses via the
query-builder API, and invoke chQuery with the built query object so escaping
and composition follow the established helpers.
Comment on lines
+212
to
+213
| await writeGroupToCh(updated); | ||
| return { ...existing, ...updated }; |
Contributor
There was a problem hiding this comment.
Return the freshly written row after update.
The object returned here keeps the old updatedAt, because updated never sets a new one and the merge reuses existing.updatedAt. Re-read the group after writeGroupToCh() or derive updatedAt from the new version timestamp before returning.
Proposed fix
await writeGroupToCh(updated);
- return { ...existing, ...updated };
+ const persisted = await getGroupById(id, projectId);
+ if (!persisted) {
+ throw new Error(`Group ${id} not found after update`);
+ }
+ return persisted;📝 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
| await writeGroupToCh(updated); | |
| return { ...existing, ...updated }; | |
| await writeGroupToCh(updated); | |
| const persisted = await getGroupById(id, projectId); | |
| if (!persisted) { | |
| throw new Error(`Group ${id} not found after update`); | |
| } | |
| return persisted; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/db/src/services/group.service.ts` around lines 212 - 213, The
returned object merges existing and updated so it preserves the old updatedAt;
after calling writeGroupToCh(updated) you should return the actual persisted row
or at least update updatedAt from the persisted version—so re-read the group
(e.g., fetch by id using the existing read method) after writeGroupToCh(updated)
and return that fresh row, or obtain the new version/timestamp from
writeGroupToCh and set updatedAt on the returned object instead of merging
existing.updatedAt.
Comment on lines
+328
to
+348
| const rows = await chQuery<{ profile_id: string; total_count: number }>(` | ||
| SELECT | ||
| gm.profile_id, | ||
| count() OVER () AS total_count | ||
| FROM ( | ||
| SELECT profile_id, max(created_at) AS last_seen | ||
| FROM ${TABLE_NAMES.events} | ||
| WHERE project_id = ${sqlstring.escape(projectId)} | ||
| AND has(groups, ${sqlstring.escape(groupId)}) | ||
| AND profile_id != device_id | ||
| GROUP BY profile_id | ||
| ) gm | ||
| INNER JOIN ( | ||
| SELECT id FROM ${TABLE_NAMES.profiles} FINAL | ||
| WHERE project_id = ${sqlstring.escape(projectId)} | ||
| ${searchCondition} | ||
| ) p ON p.id = gm.profile_id | ||
| ORDER BY gm.last_seen DESC | ||
| LIMIT ${take} | ||
| OFFSET ${offset} | ||
| `); |
Contributor
There was a problem hiding this comment.
Read membership from profiles.groups, not only from events.
handleGroup() in apps/api/src/controllers/track.controller.ts persists membership with upsertProfile({ groups: [id] }), but this query only scans events.groups. Any profile added through the group endpoint and not yet seen in a grouped event will be missing from the member list. If this endpoint is meant to show group members, the source of truth needs to include profiles.groups.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/db/src/services/group.service.ts` around lines 328 - 348, The
current member query only reads membership from events (SELECT ... FROM
${TABLE_NAMES.events}) but must also include profiles with group membership
stored in profiles.groups (set via upsertProfile in handleGroup); update the
query used in group.service.ts (the chQuery block) to union or join profiles
where profiles.groups contains the groupId (e.g., include profile IDs from
${TABLE_NAMES.profiles} where has(groups, ${sqlstring.escape(groupId)})), ensure
duplicates are deduplicated (profile_id), preserve or compute a sensible
last_seen (use max(created_at) from events when available, nullable otherwise),
and keep the existing ORDER BY/ LIMIT/ OFFSET and the filtering via
searchCondition so profiles added via upsertProfile are returned as members even
if they have no grouped events.
| type: z.string().min(1), | ||
| name: z.string().min(1), | ||
| properties: z.record(z.unknown()).optional(), | ||
| profileId: z.union([z.string().min(1), z.number()]).optional(), |
Contributor
There was a problem hiding this comment.
Normalize profileId to one output type at the validation boundary.
These schemas now accept numeric profileIds, but downstream still treats the field as truthy string-like data. A valid 0 will pass validation and later be treated as “missing” in apps/api/src/controllers/track.controller.ts when building the event groupId and when deciding whether to upsert group membership. Coerce accepted numbers to strings here, or keep the field string-only, so every consumer sees one stable type.
Proposed fix
export const zGroupPayload = z.object({
id: z.string().min(1),
type: z.string().min(1),
name: z.string().min(1),
properties: z.record(z.unknown()).optional(),
- profileId: z.union([z.string().min(1), z.number()]).optional(),
+ profileId: z
+ .union([z.string().min(1), z.number()])
+ .transform((value) => String(value))
+ .optional(),
});
export const zTrackPayload = z
.object({
name: z.string().min(1),
properties: z.record(z.string(), z.unknown()).optional(),
- profileId: z.union([z.string().min(1), z.number()]).optional(),
+ profileId: z
+ .union([z.string().min(1), z.number()])
+ .transform((value) => String(value))
+ .optional(),
groups: z.array(z.string().min(1)).optional(),
})Also applies to: 17-17
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/validation/src/track.validation.ts` at line 10, The schema for
profileId currently allows both strings and numbers which lets numeric 0 pass
validation but later be treated as missing; update the profileId definition in
track.validation.ts to normalize to a single output type (preferably string) by
replacing the union with a string-only schema or by using a preprocessing step
(e.g., z.preprocess) to coerce numbers to strings so that the exported/parsed
value for profileId is always a non-empty string for downstream code (refer to
the profileId schema and the surrounding track validation exports).
lindesvard
force-pushed
the
feature/groups
branch
from
4650452 to
4c24839
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/db/src/services/chart.service.ts (1)
348-358:⚠️ Potential issue | 🔴 CriticalBug: Missing
projectIdfor group property metrics.Line 349 calls
getSelectPropertyKey(event.property)without passingprojectId. Ifevent.propertystarts withgroup.(e.g.,group.properties.plan), the function will return an invalid column reference instead of the correct_g.properties[...]expression.Compare with line 607 in
getAggregateChartSqlwhich correctly passesprojectId.🐛 Proposed fix
if (mathFunction && event.property) { - const propertyKey = getSelectPropertyKey(event.property); + const propertyKey = getSelectPropertyKey(event.property, projectId); if (isNumericColumn(event.property)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/src/services/chart.service.ts` around lines 348 - 358, In the block that builds count/select for mathFunction you must call getSelectPropertyKey with the projectId so group.* properties are resolved correctly; replace getSelectPropertyKey(event.property) with getSelectPropertyKey(event.property, projectId) (matching the usage in getAggregateChartSql), leaving the isNumericColumn, sb.select.count and sb.where.property logic unchanged so group property references become the correct _g.properties[...] expression.
🧹 Nitpick comments (9)
packages/db/src/services/funnel.service.ts (1)
290-296: Consider using the query builder for the_gCTE definition.The raw SQL string could be replaced with the query builder for consistency:
if (needsGroupArrayJoin) { - funnelQuery.with( - '_g', - `SELECT id, name, type, properties FROM ${TABLE_NAMES.groups} FINAL WHERE project_id = ${sqlstring.escape(projectId)}`, - ); + const groupsCte = clix(this.client, timezone) + .select(['id', 'name', 'type', 'properties']) + .from(TABLE_NAMES.groups, true) // true for FINAL + .where('project_id', '=', projectId); + funnelQuery.with('_g', groupsCte); }As per coding guidelines: "When writing ClickHouse queries, always use the custom query builder from
./packages/db/src/clickhouse/query-builder.ts"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/src/services/funnel.service.ts` around lines 290 - 296, The _g CTE is being added with a raw SQL string in funnelQuery.with which violates the ClickHouse query-builder guideline; update the code so that when needsGroupArrayJoin is true you build the SELECT via the custom query builder (from ./packages/db/src/clickhouse/query-builder.ts) instead of string interpolation, e.g. construct a query selecting id, name, type, properties from TABLE_NAMES.groups with a WHERE project_id = sqlstring.escape(projectId) using the query-builder API and pass that built query to funnelQuery.with('_g', builtQuery) to maintain consistency and prevent raw SQL injection.apps/start/src/components/groups/table/index.tsx (1)
1-1: Unused import.
IServiceGroupis imported but not used in this file. OnlyIServiceGroupWithStatsfrom./columnsis used.Proposed fix
-import type { IServiceGroup } from '@openpanel/db';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/start/src/components/groups/table/index.tsx` at line 1, Remove the unused import IServiceGroup from the top of the file: keep only the import(s) actually used (e.g., IServiceGroupWithStats from ./columns) and eliminate the unused IServiceGroup symbol to clean up the module imports in index.tsx.packages/trpc/src/routers/group.ts (1)
97-135: Consider using the custom query builder instead of raw SQL.Multiple endpoints in this router use raw SQL strings with
chQueryandsqlstring.escape(). Themetrics,activity,members,mostEvents,popularRoutes, andmemberGrowthendpoints all construct queries this way.Other files in this PR (e.g.,
chart.ts) use theclix()query builder for similar ClickHouse operations. Using the query builder would provide better type safety and consistency.As per coding guidelines: "When writing ClickHouse queries, always use the custom query builder from
./packages/db/src/clickhouse/query-builder.ts".Example refactor for the metrics endpoint
metrics: protectedProcedure .input(z.object({ id: z.string(), projectId: z.string() })) .query(async ({ input: { id, projectId } }) => { - const data = await chQuery<{ - totalEvents: number; - uniqueProfiles: number; - firstSeen: string; - lastSeen: string; - }>(` - SELECT - count() AS totalEvents, - uniqExact(profile_id) AS uniqueProfiles, - min(created_at) AS firstSeen, - max(created_at) AS lastSeen - FROM ${TABLE_NAMES.events} - WHERE project_id = ${sqlstring.escape(projectId)} - AND has(groups, ${sqlstring.escape(id)}) - `); + const data = await clix(ch) + .select<{ + totalEvents: number; + uniqueProfiles: number; + firstSeen: string; + lastSeen: string; + }>([ + 'count() AS totalEvents', + 'uniqExact(profile_id) AS uniqueProfiles', + 'min(created_at) AS firstSeen', + 'max(created_at) AS lastSeen', + ]) + .from(TABLE_NAMES.events) + .where('project_id', '=', projectId) + .whereRaw(`has(groups, ${sqlstring.escape(id)})`) + .execute();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/trpc/src/routers/group.ts` around lines 97 - 135, The router uses raw SQL via chQuery and sqlstring.escape in the metrics and activity handlers (and other endpoints) which violates the guideline to use the custom ClickHouse query builder; replace those raw-string queries with the clix() query-builder calls from ./packages/db/src/clickhouse/query-builder.ts, building equivalent SELECTs (count(), uniqExact(profile_id), min/max(created_at), toStartOfDay(created_at), GROUP BY/ORDER BY) and parameterizing projectId and id via the builder instead of sqlstring.escape; update the metrics and activity procedures (and similarly members, mostEvents, popularRoutes, memberGrowth) to call clix().toString() or the builder's execution helper to produce the same result shape expected by chQuery while preserving the return mapping (totalEvents, uniqueProfiles, firstSeen, lastSeen) and types.apps/start/src/routes/_app.$organizationId.$projectId.groups_.$groupId._tabs.index.tsx (1)
24-32: Consider prefetching more queries in the loader.The loader only prefetches
group.activity, but the component suspends on 7 different queries (byId,metrics,activity,members,mostEvents,popularRoutes,memberGrowth). This could cause waterfall requests during SSR/hydration.Suggested loader enhancement
loader: async ({ context, params }) => { await Promise.all([ + context.queryClient.prefetchQuery( + context.trpc.group.byId.queryOptions({ + id: params.groupId, + projectId: params.projectId, + }) + ), + context.queryClient.prefetchQuery( + context.trpc.group.metrics.queryOptions({ + id: params.groupId, + projectId: params.projectId, + }) + ), context.queryClient.prefetchQuery( context.trpc.group.activity.queryOptions({ id: params.groupId, projectId: params.projectId, }) ), + context.queryClient.prefetchQuery( + context.trpc.group.members.queryOptions({ + id: params.groupId, + projectId: params.projectId, + }) + ), ]); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/start/src/routes/_app`.$organizationId.$projectId.groups_.$groupId._tabs.index.tsx around lines 24 - 32, The loader currently only prefetches group.activity causing potential waterfalls; update the loader (the async loader function in _tabs.index.tsx) to also prefetch the other queries the component suspends on: context.trpc.group.byId, context.trpc.group.metrics, context.trpc.group.members, context.trpc.group.mostEvents, context.trpc.group.popularRoutes, and context.trpc.group.memberGrowth (all with the same params: id: params.groupId, projectId: params.projectId) by adding them into the Promise.all array so all seven queries are prefetched in parallel before rendering.apps/testbed/src/main.tsx (1)
6-10: Consider wrapping with<React.StrictMode>for development benefits.StrictMode helps catch common bugs during development by running certain checks and warnings.
♻️ Optional enhancement
+import { StrictMode } from 'react'; import { createRoot } from 'react-dom/client'; import { BrowserRouter } from 'react-router-dom'; import App from './App'; import './styles.css'; createRoot(document.getElementById('root')!).render( + <StrictMode> <BrowserRouter> <App /> </BrowserRouter> + </StrictMode> );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/testbed/src/main.tsx` around lines 6 - 10, Wrap the rendered app with React.StrictMode to enable development-only checks: update the createRoot(...).render call so that the <BrowserRouter><App /></BrowserRouter> tree is wrapped inside <React.StrictMode> (ensure React is imported if not already and keep the existing createRoot(document.getElementById('root')!) usage intact); this change targets the render invocation around the BrowserRouter/App components to enable StrictMode during development.apps/testbed/src/styles.css (1)
305-308: Duplicate.product-card-nameselector.This selector is already defined at lines 159-161. Consider consolidating into a single rule to avoid maintenance confusion.
♻️ Proposed consolidation
Remove lines 305-308 and update the original rule:
.product-card-name { font-weight: bold; + color: inherit; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/testbed/src/styles.css` around lines 305 - 308, There’s a duplicate CSS rule for the .product-card-name selector; remove the second block and consolidate its declarations into the original .product-card-name rule (ensure font-weight: bold and color: inherit are present on the primary rule), then delete the redundant selector to avoid maintenance confusion.apps/testbed/src/pages/Login.tsx (1)
95-97: Consider narrowing the type forset()to prevent misuse.The current signature allows setting
groupIds(astring[]) with astringvalue. While the actual usage is correct, stricter typing would prevent future bugs.♻️ Proposed improvement
- function set(field: keyof User, value: string) { + function set(field: 'id' | 'firstName' | 'lastName' | 'email', value: string) { setForm((prev) => ({ ...prev, [field]: value })); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/testbed/src/pages/Login.tsx` around lines 95 - 97, The setter currently accepts (field: keyof User, value: string) which allows assigning a string to User.groupIds (string[]); change the signature to be generic so the value type matches the selected key (e.g., function set<K extends keyof User>(field: K, value: User[K]) ) and update call sites accordingly so TypeScript enforces correct types for keys like groupIds; keep references to set, User and setForm in Login.tsx when making this change.apps/testbed/src/App.tsx (1)
178-212: Consider extracting success/error pages into separate components.The inline JSX for
/successand/errorroutes is verbose. Extracting them toSuccessPageandErrorPagecomponents would improve readability and enable reuse.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/testbed/src/App.tsx` around lines 178 - 212, The inline JSX for the /success and /error Route elements makes App.tsx noisy; create two new components (e.g., SuccessPage and ErrorPage) that render the respective markup and replace the current Route element props with <SuccessPage /> and <ErrorPage />; export the components (or define them in the same file above the router), move the result-page/result-icon/result-title/result-actions markup into those components, import them where App uses <Route element={...}>, and ensure the Route paths remain "/success" and "/error" and that any Link targets and button types are preserved.apps/start/src/components/ui/select.tsx (1)
35-39: Make the built-insizevariants distinct.On Line 35, both
data-[size=default]anddata-[size=sm]map toh-8, so thedata-size={size}hook on Line 38 does not create any visual difference inside this component. If this API is meant to expose two trigger sizes,defaultshould get its own height again.Possible adjustment
- "flex w-fit items-center justify-between gap-2 whitespace-nowrap rounded-md border border-input bg-transparent px-3 py-2 text-sm shadow-xs outline-none transition-[color,box-shadow] focus-visible:border-ring focus-visible:ring-[3px] focus-visible:ring-ring/50 disabled:cursor-not-allowed disabled:opacity-50 aria-invalid:border-destructive aria-invalid:ring-destructive/20 data-[size=default]:h-8 data-[size=sm]:h-8 data-[placeholder]:text-muted-foreground *:data-[slot=select-value]:line-clamp-1 *:data-[slot=select-value]:flex *:data-[slot=select-value]:items-center *:data-[slot=select-value]:gap-2 dark:bg-input/30 dark:aria-invalid:ring-destructive/40 dark:hover:bg-input/50 [&_svg:not([class*='size-'])]:size-4 [&_svg:not([class*='text-'])]:text-muted-foreground [&_svg]:pointer-events-none [&_svg]:shrink-0", + "flex w-fit items-center justify-between gap-2 whitespace-nowrap rounded-md border border-input bg-transparent px-3 py-2 text-sm shadow-xs outline-none transition-[color,box-shadow] focus-visible:border-ring focus-visible:ring-[3px] focus-visible:ring-ring/50 disabled:cursor-not-allowed disabled:opacity-50 aria-invalid:border-destructive aria-invalid:ring-destructive/20 data-[size=default]:h-9 data-[size=sm]:h-8 data-[placeholder]:text-muted-foreground *:data-[slot=select-value]:line-clamp-1 *:data-[slot=select-value]:flex *:data-[slot=select-value]:items-center *:data-[slot=select-value]:gap-2 dark:bg-input/30 dark:aria-invalid:ring-destructive/40 dark:hover:bg-input/50 [&_svg:not([class*='size-'])]:size-4 [&_svg:not([class*='text-'])]:text-muted-foreground [&_svg]:pointer-events-none [&_svg]:shrink-0",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/start/src/components/ui/select.tsx` around lines 35 - 39, The select trigger's size variants are not distinct because both selectors data-[size=default] and data-[size=sm] currently set h-8; update the classes used when building the component's className so the data-size prop produces different heights (e.g., change the data-[size=default] rule to a larger height than data-[size=sm]). Locate the class string in the Select component where data-[size=default]:h-8 and data-[size=sm]:h-8 are defined and adjust the default variant (via data-[size=default]) to the desired height (for example h-10) so data-size={size} has a visual effect. Ensure any related spacing or alignment rules still work with the new height.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/start/src/components/profiles/popular-routes.tsx`:
- Line 14: Update the user-facing title text in the Popular Routes component:
locate the JSX element using WidgetTitle in popular-routes.tsx (the line
rendering <WidgetTitle>Most visted pages</WidgetTitle>) and correct the typo
from "visted" to "visited" so it reads "Most visited pages".
In `@apps/start/src/components/report/sidebar/PropertiesCombobox.tsx`:
- Line 210: The VirtualList is using itemKey="id" but the data objects
(eventActions, profileActions) only have value/label/description, causing
undefined keys; update the VirtualList itemKey to "value" (to match renderGroup
which already uses itemKey="value") for the VirtualList instances that render
eventActions/profileActions (locate the VirtualList prop itemKey and change it
to "value") so keys are stable and consistent with the data shape and
renderGroup.
In `@apps/testbed/src/App.tsx`:
- Around line 24-38: The JSON.parse call inside the useEffect can throw if
localStorage has corrupted data; wrap the parsing and subsequent logic in a
try/catch around the block that reads stored =
localStorage.getItem('op_testbed_user') and JSON.parse(stored) in the useEffect,
and on catch log the error, clear or remove the corrupted key
(localStorage.removeItem('op_testbed_user')), and proceed to call op.ready() so
the app doesn't crash; ensure you only call op.identify, setUser, and
applyGroups when parsing succeeds.
- Around line 40-48: applyGroups currently calls non-existent SDK methods
op.setGroups and op.upsertGroup; replace these with supported API calls: gather
the PRESET_GROUPS metadata for u.groupIds, then call op.identify({ id: u.id,
traits: { groups: collectedGroups } }) to set the user's groups as traits (or
op.setGlobalProperties if you want global context), and if you need per-group
upsert semantics emit an event per group via op.track('group_upsert', { userId:
u.id, group: meta }) inside the same loop instead of op.upsertGroup; update
references to applyGroups, op.setGroups and op.upsertGroup accordingly.
In `@apps/testbed/src/pages/Login.tsx`:
- Line 173: The JSX is accessing a non-existent top-level property `group.plan`
(in the span with className "group-plan"); update the access to use the nested
structure `group.properties.plan` so the rendered plan shows the correct value
from the PRESET_GROUPS shape.
In `@apps/testbed/src/styles.css`:
- Line 329: Change the CSS keyword to lowercase to satisfy Stylelint's
value-keyword-case rule: update the border-left declaration that currently uses
"currentColor" (or "currentColor" variation) to "currentcolor" so the property
"border-left: 1px solid currentcolor;" uses the lowercase CSS keyword
consistently.
In `@packages/db/index.ts`:
- Around line 5-6: The export list duplicates the same module: remove the
redundant export line "export * from './src/engine';" so the file only
re-exports './src/engine' once; locate the two identical export statements in
the module and delete one to avoid duplicate exports and any linter warnings.
In `@packages/db/src/buffers/group-buffer.ts`:
- Around line 90-141: The add() method currently does a read-modify-write
without synchronization causing lost updates for the same group; wrap the
per-group read/merge/write sequence in a short per-group lock (e.g., using a
Redis lock key derived from getCacheKey(projectId, id) or a separate lock key
like `${cacheKey}:lock`) to serialize concurrent callers: acquire lock with SET
NX and expiration, re-read the cache inside the lock
(fetchFromCache/fetchFromClickhouse), perform the merge and the multi() writes,
then release the lock in a finally block and retry/backoff a few times on lock
contention; ensure the lock is held only for the minimal time and handle
lock-acquire failures by retrying or returning an error.
- Around line 63-86: The cache key and ClickHouse lookup must include the
group's type to avoid collisions between different entity types; update
getCacheKey to accept a type parameter and include it in the returned string
(e.g. `${this.redisCachePrefix}${projectId}:${type}:${id}`), update
fetchFromCache signature to accept type and call getCacheKey(projectId, type,
id), and update fetchFromClickhouse to accept type and add AND type =
${sqlstring.escape(type)} to the WHERE clause (also ensure SELECT still returns
type so IGroupCacheEntry remains valid); adjust calls to chQuery/getSafeJson
accordingly so all lookups and cache writes are keyed by (project_id, type, id).
- Around line 76-86: Replace the inline SQL in fetchFromClickhouse (which
currently calls chQuery with a template string selecting from
TABLE_NAMES.groups) with the project's ClickHouse query builder utilities:
import the select/from builders and query-functions, build a SELECT for columns
(project_id, id, type, name, properties, created_at) from TABLE_NAMES.groups,
add WHERE conditions for project_id, id and deleted = 0 using the
query-functions (so values are handled/escaped consistently), convert the built
query to SQL and pass that to chQuery, and keep the function signature and
return type IGroupCacheEntry | null unchanged.
In `@packages/db/src/buffers/profile-buffer.ts`:
- Around line 143-152: The backfill/select query that populates existingProfile
is not selecting the groups column, causing mergedProfile.groups to fall back to
undefined and later overwrite stored groups; update the ClickHouse
backfill/select query used when loading existingProfile (the query that feeds
into the code which constructs mergedProfile in profile-buffer.ts) to include
the groups column so existingProfile.groups is populated, and apply the same fix
to the other occurrence referenced around the 240-267 block so both merges
preserve existing group memberships.
- Around line 116-203: The per-profile lock held by withProfileLock (wrapper
used around the write code) currently remains held across the call to
tryFlush(), which can block and allow the lock TTL to expire; fix by moving the
tryFlush() call outside the withProfileLock callback: inside the callback (in
the function passed to withProfileLock) perform the fetch/merge, the Redis
multi().exec(), capture the resulting bufferLength (from result[3][1]) and any
relevant cacheKey/profile info, then return that bufferLength (or set a local
variable). After the withProfileLock completes (lock released), check the
captured bufferLength against this.batchSize and call this.tryFlush() only if >=
batchSize. Ensure the function names referenced are withProfileLock, tryFlush,
getProfileCacheKey, and the redis multi/exec handling that computes
bufferLength.
- Around line 61-92: withProfileLock currently falls back to calling fn()
unlocked after maxRetries, reintroducing lost-update races; change it to throw a
clear error instead of executing fn() when the lock cannot be acquired. In the
withProfileLock method (referencing lockKey, lockId, maxRetries, retryDelayMs,
releaseLockSha, releaseLockScript), remove the final call to return fn() and
instead log the failure and throw an Error (or a custom LockAcquisitionError) so
callers must handle/retry upstream; ensure the error includes profileId and
projectId for observability.
In `@scripts/seed-events.mjs`:
- Around line 453-457: sendEvent() currently only logs non-2xx responses and
still returns the Response, causing callers to treat failures as successes;
change sendEvent (the function that contains the if (!res.ok) block) to treat
non-OK responses as errors by rejecting/throwing: after reading the response
text (const text = await res.text().catch(() => '')), throw a new Error that
includes res.status, payload.type/payload.payload?.name, and a snippet of the
text so callers see the failure and increment error counts instead of counting
it as success. Ensure the function no longer returns a successful Response on
non-OK status.
- Around line 178-180: The purchase and checkout_success event objects are each
calling Date.now() separately, producing mismatched order IDs; fix by generating
a single orderId variable (e.g., const orderId = `ord_${Date.now()}`) for that
simulated checkout flow and reuse orderId in both the purchase and
checkout_success props (and any other related events like those groups where
'purchase' and 'checkout_success' are created); update all similar blocks (the
repeated checkout event groups) to follow this pattern so all events in a flow
share the same order_id.
- Around line 486-499: The seeded event timestamps can exceed "now" because
sessionStart is computed across the timeline and then events add
sessionDurationMs, so ensure sessions end inside the requested window by
subtracting the session duration when computing start: compute sessionDurationMs
first, then set sessionStart = now - timelineMs + sessionStartOffset -
sessionDurationMs (or clamp sessionStart so sessionStart + sessionDurationMs <=
now); update references to sessionStartOffset, sessionStart, sessionDurationMs
and eventOffset accordingly so the last event timestamp never exceeds now.
---
Outside diff comments:
In `@packages/db/src/services/chart.service.ts`:
- Around line 348-358: In the block that builds count/select for mathFunction
you must call getSelectPropertyKey with the projectId so group.* properties are
resolved correctly; replace getSelectPropertyKey(event.property) with
getSelectPropertyKey(event.property, projectId) (matching the usage in
getAggregateChartSql), leaving the isNumericColumn, sb.select.count and
sb.where.property logic unchanged so group property references become the
correct _g.properties[...] expression.
---
Nitpick comments:
In `@apps/start/src/components/groups/table/index.tsx`:
- Line 1: Remove the unused import IServiceGroup from the top of the file: keep
only the import(s) actually used (e.g., IServiceGroupWithStats from ./columns)
and eliminate the unused IServiceGroup symbol to clean up the module imports in
index.tsx.
In `@apps/start/src/components/ui/select.tsx`:
- Around line 35-39: The select trigger's size variants are not distinct because
both selectors data-[size=default] and data-[size=sm] currently set h-8; update
the classes used when building the component's className so the data-size prop
produces different heights (e.g., change the data-[size=default] rule to a
larger height than data-[size=sm]). Locate the class string in the Select
component where data-[size=default]:h-8 and data-[size=sm]:h-8 are defined and
adjust the default variant (via data-[size=default]) to the desired height (for
example h-10) so data-size={size} has a visual effect. Ensure any related
spacing or alignment rules still work with the new height.
In
`@apps/start/src/routes/_app`.$organizationId.$projectId.groups_.$groupId._tabs.index.tsx:
- Around line 24-32: The loader currently only prefetches group.activity causing
potential waterfalls; update the loader (the async loader function in
_tabs.index.tsx) to also prefetch the other queries the component suspends on:
context.trpc.group.byId, context.trpc.group.metrics, context.trpc.group.members,
context.trpc.group.mostEvents, context.trpc.group.popularRoutes, and
context.trpc.group.memberGrowth (all with the same params: id: params.groupId,
projectId: params.projectId) by adding them into the Promise.all array so all
seven queries are prefetched in parallel before rendering.
In `@apps/testbed/src/App.tsx`:
- Around line 178-212: The inline JSX for the /success and /error Route elements
makes App.tsx noisy; create two new components (e.g., SuccessPage and ErrorPage)
that render the respective markup and replace the current Route element props
with <SuccessPage /> and <ErrorPage />; export the components (or define them in
the same file above the router), move the
result-page/result-icon/result-title/result-actions markup into those
components, import them where App uses <Route element={...}>, and ensure the
Route paths remain "/success" and "/error" and that any Link targets and button
types are preserved.
In `@apps/testbed/src/main.tsx`:
- Around line 6-10: Wrap the rendered app with React.StrictMode to enable
development-only checks: update the createRoot(...).render call so that the
<BrowserRouter><App /></BrowserRouter> tree is wrapped inside <React.StrictMode>
(ensure React is imported if not already and keep the existing
createRoot(document.getElementById('root')!) usage intact); this change targets
the render invocation around the BrowserRouter/App components to enable
StrictMode during development.
In `@apps/testbed/src/pages/Login.tsx`:
- Around line 95-97: The setter currently accepts (field: keyof User, value:
string) which allows assigning a string to User.groupIds (string[]); change the
signature to be generic so the value type matches the selected key (e.g.,
function set<K extends keyof User>(field: K, value: User[K]) ) and update call
sites accordingly so TypeScript enforces correct types for keys like groupIds;
keep references to set, User and setForm in Login.tsx when making this change.
In `@apps/testbed/src/styles.css`:
- Around line 305-308: There’s a duplicate CSS rule for the .product-card-name
selector; remove the second block and consolidate its declarations into the
original .product-card-name rule (ensure font-weight: bold and color: inherit
are present on the primary rule), then delete the redundant selector to avoid
maintenance confusion.
In `@packages/db/src/services/funnel.service.ts`:
- Around line 290-296: The _g CTE is being added with a raw SQL string in
funnelQuery.with which violates the ClickHouse query-builder guideline; update
the code so that when needsGroupArrayJoin is true you build the SELECT via the
custom query builder (from ./packages/db/src/clickhouse/query-builder.ts)
instead of string interpolation, e.g. construct a query selecting id, name,
type, properties from TABLE_NAMES.groups with a WHERE project_id =
sqlstring.escape(projectId) using the query-builder API and pass that built
query to funnelQuery.with('_g', builtQuery) to maintain consistency and prevent
raw SQL injection.
In `@packages/trpc/src/routers/group.ts`:
- Around line 97-135: The router uses raw SQL via chQuery and sqlstring.escape
in the metrics and activity handlers (and other endpoints) which violates the
guideline to use the custom ClickHouse query builder; replace those raw-string
queries with the clix() query-builder calls from
./packages/db/src/clickhouse/query-builder.ts, building equivalent SELECTs
(count(), uniqExact(profile_id), min/max(created_at), toStartOfDay(created_at),
GROUP BY/ORDER BY) and parameterizing projectId and id via the builder instead
of sqlstring.escape; update the metrics and activity procedures (and similarly
members, mostEvents, popularRoutes, memberGrowth) to call clix().toString() or
the builder's execution helper to produce the same result shape expected by
chQuery while preserving the return mapping (totalEvents, uniqueProfiles,
firstSeen, lastSeen) and types.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 38278e45-3b2d-4122-a896-1058a230c985
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (89)
apps/api/scripts/test.tsapps/api/src/controllers/track.controller.tsapps/public/content/docs/dashboard/understand-the-overview.mdxapps/start/src/components/events/table/columns.tsxapps/start/src/components/groups/group-member-growth.tsxapps/start/src/components/groups/table/columns.tsxapps/start/src/components/groups/table/index.tsxapps/start/src/components/organization/billing-usage.tsxapps/start/src/components/profiles/most-events.tsxapps/start/src/components/profiles/popular-routes.tsxapps/start/src/components/profiles/profile-groups.tsxapps/start/src/components/profiles/table/columns.tsxapps/start/src/components/report-chart/funnel/chart.tsxapps/start/src/components/report/ReportSegment.tsxapps/start/src/components/report/sidebar/PropertiesCombobox.tsxapps/start/src/components/sessions/table/columns.tsxapps/start/src/components/sidebar-project-menu.tsxapps/start/src/components/ui/data-table/data-table-toolbar.tsxapps/start/src/components/ui/select.tsxapps/start/src/components/widget.tsxapps/start/src/modals/add-group.tsxapps/start/src/modals/edit-group.tsxapps/start/src/modals/index.tsxapps/start/src/modals/view-chart-users.tsxapps/start/src/routeTree.gen.tsapps/start/src/routes/_app.$organizationId.$projectId.groups.tsxapps/start/src/routes/_app.$organizationId.$projectId.groups_.$groupId._tabs.events.tsxapps/start/src/routes/_app.$organizationId.$projectId.groups_.$groupId._tabs.index.tsxapps/start/src/routes/_app.$organizationId.$projectId.groups_.$groupId._tabs.members.tsxapps/start/src/routes/_app.$organizationId.$projectId.groups_.$groupId._tabs.tsxapps/start/src/routes/_app.$organizationId.$projectId.profiles.$profileId._tabs.index.tsxapps/start/src/routes/_app.$organizationId.$projectId.sessions_.$sessionId.tsxapps/start/src/utils/title.tsapps/testbed/.gitignoreapps/testbed/index.htmlapps/testbed/package.jsonapps/testbed/scripts/copy-op1.mjsapps/testbed/src/App.tsxapps/testbed/src/analytics.tsapps/testbed/src/main.tsxapps/testbed/src/pages/Cart.tsxapps/testbed/src/pages/Checkout.tsxapps/testbed/src/pages/Login.tsxapps/testbed/src/pages/Product.tsxapps/testbed/src/pages/Shop.tsxapps/testbed/src/styles.cssapps/testbed/src/types.tsapps/testbed/tsconfig.jsonapps/testbed/vite.config.tsapps/worker/src/boot-cron.tsapps/worker/src/jobs/cron.tsapps/worker/src/jobs/events.incoming-event.tsapps/worker/src/jobs/events.incoming-events.test.tsbiome.jsondocker-compose.ymlpackages/common/scripts/get-referrers.tspackages/constants/index.tspackages/db/code-migrations/11-add-groups.tspackages/db/index.tspackages/db/prisma/schema.prismapackages/db/src/buffers/event-buffer.tspackages/db/src/buffers/group-buffer.tspackages/db/src/buffers/index.tspackages/db/src/buffers/profile-buffer.test.tspackages/db/src/buffers/profile-buffer.tspackages/db/src/buffers/session-buffer.tspackages/db/src/clickhouse/client.tspackages/db/src/clickhouse/query-builder.tspackages/db/src/engine/index.tspackages/db/src/services/chart.service.tspackages/db/src/services/conversion.service.tspackages/db/src/services/event.service.tspackages/db/src/services/funnel.service.tspackages/db/src/services/group.service.tspackages/db/src/services/import.service.tspackages/db/src/services/profile.service.tspackages/db/src/services/session.service.tspackages/importer/src/providers/mixpanel.test.tspackages/importer/src/providers/mixpanel.tspackages/importer/src/providers/umami.tspackages/logger/index.tspackages/queue/src/queues.tspackages/sdks/sdk/src/index.tspackages/trpc/src/root.tspackages/trpc/src/routers/chart.tspackages/trpc/src/routers/event.tspackages/trpc/src/routers/group.tspackages/validation/src/track.validation.tsscripts/seed-events.mjs
🚧 Files skipped from review as they are similar to previous changes (42)
- apps/start/src/components/groups/group-member-growth.tsx
- packages/importer/src/providers/mixpanel.test.ts
- apps/testbed/src/pages/Shop.tsx
- apps/testbed/src/pages/Checkout.tsx
- apps/start/src/modals/view-chart-users.tsx
- packages/db/src/clickhouse/client.ts
- apps/start/src/components/profiles/profile-groups.tsx
- apps/start/src/modals/edit-group.tsx
- packages/db/src/buffers/session-buffer.ts
- apps/start/src/routes/app.$organizationId.$projectId.groups.$groupId._tabs.members.tsx
- apps/start/src/components/ui/data-table/data-table-toolbar.tsx
- packages/db/code-migrations/11-add-groups.ts
- packages/queue/src/queues.ts
- apps/testbed/package.json
- packages/trpc/src/root.ts
- apps/testbed/src/pages/Product.tsx
- apps/start/src/components/sidebar-project-menu.tsx
- packages/common/scripts/get-referrers.ts
- packages/db/src/services/session.service.ts
- docker-compose.yml
- apps/testbed/scripts/copy-op1.mjs
- apps/testbed/.gitignore
- packages/logger/index.ts
- apps/start/src/components/report/ReportSegment.tsx
- packages/db/prisma/schema.prisma
- apps/testbed/src/pages/Cart.tsx
- apps/start/src/components/events/table/columns.tsx
- apps/start/src/modals/add-group.tsx
- apps/start/src/routes/_app.$organizationId.$projectId.groups.tsx
- packages/db/src/buffers/event-buffer.ts
- apps/start/src/modals/index.tsx
- packages/db/src/engine/index.ts
- apps/api/scripts/test.ts
- packages/db/src/services/profile.service.ts
- apps/start/src/routes/_app.$organizationId.$projectId.profiles.$profileId._tabs.index.tsx
- packages/trpc/src/routers/event.ts
- packages/db/src/services/import.service.ts
- apps/worker/src/jobs/events.incoming-event.ts
- apps/start/src/routes/app.$organizationId.$projectId.groups.$groupId._tabs.tsx
- apps/api/src/controllers/track.controller.ts
- apps/testbed/index.html
- apps/testbed/tsconfig.json
| return ( | ||
| <Widget className="w-full"> | ||
| <WidgetHead> | ||
| <WidgetTitle>Most visted pages</WidgetTitle> |
Contributor
There was a problem hiding this comment.
Typo in user-facing text: "visted" → "visited"
📝 Proposed fix
- <WidgetTitle>Most visted pages</WidgetTitle>
+ <WidgetTitle>Most visited pages</WidgetTitle>📝 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
| <WidgetTitle>Most visted pages</WidgetTitle> | |
| <WidgetTitle>Most visited pages</WidgetTitle> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/start/src/components/profiles/popular-routes.tsx` at line 14, Update the
user-facing title text in the Popular Routes component: locate the JSX element
using WidgetTitle in popular-routes.tsx (the line rendering <WidgetTitle>Most
visted pages</WidgetTitle>) and correct the typo from "visted" to "visited" so
it reads "Most visited pages".
| data={filteredActions} | ||
| height={300} | ||
| itemHeight={40} | ||
| itemKey="id" |
Contributor
There was a problem hiding this comment.
VirtualList itemKey references non-existent property.
The eventActions and profileActions objects have properties { value, label, description }, but the VirtualList uses itemKey="id" which doesn't exist. This causes undefined keys and potential rendering issues. The renderGroup function correctly uses itemKey="value" (line 288).
🐛 Proposed fix
At line 210:
<VirtualList
data={filteredActions}
height={300}
itemHeight={40}
- itemKey="id"
+ itemKey="value"
>At line 249:
<VirtualList
data={filteredActions}
height={300}
itemHeight={40}
- itemKey="id"
+ itemKey="value"
>Also applies to: 249-249
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/start/src/components/report/sidebar/PropertiesCombobox.tsx` at line 210,
The VirtualList is using itemKey="id" but the data objects (eventActions,
profileActions) only have value/label/description, causing undefined keys;
update the VirtualList itemKey to "value" (to match renderGroup which already
uses itemKey="value") for the VirtualList instances that render
eventActions/profileActions (locate the VirtualList prop itemKey and change it
to "value") so keys are stable and consistent with the data shape and
renderGroup.
Comment on lines
+24
to
+38
| useEffect(() => { | ||
| const stored = localStorage.getItem('op_testbed_user'); | ||
| if (stored) { | ||
| const u = JSON.parse(stored) as User; | ||
| setUser(u); | ||
| op.identify({ | ||
| profileId: u.id, | ||
| firstName: u.firstName, | ||
| lastName: u.lastName, | ||
| email: u.email, | ||
| }); | ||
| applyGroups(u); | ||
| } | ||
| op.ready(); | ||
| }, []); |
Contributor
There was a problem hiding this comment.
Add error handling for JSON.parse to prevent crashes from corrupted localStorage.
If localStorage contains malformed JSON, JSON.parse will throw and crash the app on load.
🛡️ Proposed fix
useEffect(() => {
const stored = localStorage.getItem('op_testbed_user');
if (stored) {
- const u = JSON.parse(stored) as User;
- setUser(u);
- op.identify({
- profileId: u.id,
- firstName: u.firstName,
- lastName: u.lastName,
- email: u.email,
- });
- applyGroups(u);
+ try {
+ const u = JSON.parse(stored) as User;
+ setUser(u);
+ op.identify({
+ profileId: u.id,
+ firstName: u.firstName,
+ lastName: u.lastName,
+ email: u.email,
+ });
+ applyGroups(u);
+ } catch {
+ localStorage.removeItem('op_testbed_user');
+ }
}
op.ready();
}, []);📝 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
| useEffect(() => { | |
| const stored = localStorage.getItem('op_testbed_user'); | |
| if (stored) { | |
| const u = JSON.parse(stored) as User; | |
| setUser(u); | |
| op.identify({ | |
| profileId: u.id, | |
| firstName: u.firstName, | |
| lastName: u.lastName, | |
| email: u.email, | |
| }); | |
| applyGroups(u); | |
| } | |
| op.ready(); | |
| }, []); | |
| useEffect(() => { | |
| const stored = localStorage.getItem('op_testbed_user'); | |
| if (stored) { | |
| try { | |
| const u = JSON.parse(stored) as User; | |
| setUser(u); | |
| op.identify({ | |
| profileId: u.id, | |
| firstName: u.firstName, | |
| lastName: u.lastName, | |
| email: u.email, | |
| }); | |
| applyGroups(u); | |
| } catch { | |
| localStorage.removeItem('op_testbed_user'); | |
| } | |
| } | |
| op.ready(); | |
| }, []); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/testbed/src/App.tsx` around lines 24 - 38, The JSON.parse call inside
the useEffect can throw if localStorage has corrupted data; wrap the parsing and
subsequent logic in a try/catch around the block that reads stored =
localStorage.getItem('op_testbed_user') and JSON.parse(stored) in the useEffect,
and on catch log the error, clear or remove the corrupted key
(localStorage.removeItem('op_testbed_user')), and proceed to call op.ready() so
the app doesn't crash; ensure you only call op.identify, setUser, and
applyGroups when parsing succeeds.
Comment on lines
+40
to
+48
| function applyGroups(u: User) { | ||
| op.setGroups(u.groupIds); | ||
| for (const id of u.groupIds) { | ||
| const meta = PRESET_GROUPS.find((g) => g.id === id); | ||
| if (meta) { | ||
| op.upsertGroup({ id, ...meta }); | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for setGroups and upsertGroup method definitions in the `@openpanel/web` package
echo "=== Searching for setGroups definition ==="
rg -n "setGroups" --type ts --type js -g '!node_modules' -g '**/packages/sdk*/**'
echo ""
echo "=== Searching for upsertGroup definition ==="
rg -n "upsertGroup" --type ts --type js -g '!node_modules' -g '**/packages/sdk*/**'Repository: Openpanel-dev/openpanel
Length of output: 1812
🌐 Web query:
@openpanel/web setGroups upsertGroup API methods
💡 Result:
@openpanel/web (OpenPanel Analytics Web SDK) does not include setGroups or upsertGroup API methods—those names don’t appear in the Web SDK docs or the SDK source for the exported OpenPanel class. [1][2][3]
If you’re looking at the setGroups method specifically, that’s from Atlassian’s Teamwork Graph Connector SDK, not OpenPanel. [4]
What @openpanel/web does provide is the standard OpenPanel tracking API (e.g., track, identify, setGlobalProperties, auto screen_view tracking, etc.). [1][2][3]
Sources:
[1] (openpanel.dev)
[2] (openpanel.dev)
[3] (github.com)
[4] (developer.atlassian.com)
Citations:
- 1: https://openpanel.dev/docs/sdks/web
- 2: https://openpanel.dev/docs/sdks/javascript
- 3: https://github.com/Openpanel-dev/openpanel/blob/main/packages/sdks/web/src/index.ts
- 4: https://developer.atlassian.com/platform/teamwork-graph/connector-sdk/group-operations/set-groups/?utm_source=openai
setGroups and upsertGroup do not exist in the @openpanel/web API. The @openpanel/web SDK does not provide these methods. The Web SDK only includes standard tracking methods like track, identify, setGlobalProperties, and auto screen_view tracking. Either implement these methods in the SDK or replace these calls with appropriate existing API methods.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/testbed/src/App.tsx` around lines 40 - 48, applyGroups currently calls
non-existent SDK methods op.setGroups and op.upsertGroup; replace these with
supported API calls: gather the PRESET_GROUPS metadata for u.groupIds, then call
op.identify({ id: u.id, traits: { groups: collectedGroups } }) to set the user's
groups as traits (or op.setGlobalProperties if you want global context), and if
you need per-group upsert semantics emit an event per group via
op.track('group_upsert', { userId: u.id, group: meta }) inside the same loop
instead of op.upsertGroup; update references to applyGroups, op.setGroups and
op.upsertGroup accordingly.
| type="button" | ||
| > | ||
| {group.name} | ||
| <span className="group-plan">{group.plan}</span> |
Contributor
There was a problem hiding this comment.
Incorrect property access: group.plan should be group.properties.plan.
Based on the PRESET_GROUPS structure (lines 4-65), each group has properties: { plan: string }, not a top-level plan property. This will result in undefined being rendered.
🐛 Proposed fix
- <span className="group-plan">{group.plan}</span>
+ <span className="group-plan">{group.properties.plan}</span>📝 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
| <span className="group-plan">{group.plan}</span> | |
| <span className="group-plan">{group.properties.plan}</span> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/testbed/src/pages/Login.tsx` at line 173, The JSX is accessing a
non-existent top-level property `group.plan` (in the span with className
"group-plan"); update the access to use the nested structure
`group.properties.plan` so the rendered plan shows the correct value from the
PRESET_GROUPS shape.
Comment on lines
+116
to
203
| await this.withProfileLock(profile.id, profile.project_id, async () => { | ||
| const existingProfile = await this.fetchProfile(profile, logger); | ||
|
|
||
| // Delete any properties that are not server related if we have a non-server profile | ||
| if ( | ||
| existingProfile?.properties.device !== 'server' && | ||
| profile.properties.device === 'server' | ||
| ) { | ||
| profile.properties = omit( | ||
| [ | ||
| 'city', | ||
| 'country', | ||
| 'region', | ||
| 'longitude', | ||
| 'latitude', | ||
| 'os', | ||
| 'osVersion', | ||
| 'browser', | ||
| 'device', | ||
| 'isServer', | ||
| 'os_version', | ||
| 'browser_version', | ||
| ], | ||
| profile.properties | ||
| ); | ||
| } | ||
|
|
||
| const mergedProfile: IClickhouseProfile = existingProfile | ||
| ? deepMergeObjects(existingProfile, omit(['created_at'], profile)) | ||
| : profile; | ||
| const mergedProfile: IClickhouseProfile = existingProfile | ||
| ? { | ||
| ...deepMergeObjects( | ||
| existingProfile, | ||
| omit(['created_at', 'groups'], profile) | ||
| ), | ||
| groups: uniq([ | ||
| ...(existingProfile.groups ?? []), | ||
| ...(profile.groups ?? []), | ||
| ]), | ||
| } | ||
| : profile; | ||
|
|
||
| if (profile && existingProfile) { | ||
| if ( | ||
| profile && | ||
| existingProfile && | ||
| shallowEqual( | ||
| omit(['created_at'], existingProfile), | ||
| omit(['created_at'], mergedProfile), | ||
| omit(['created_at'], mergedProfile) | ||
| ) | ||
| ) { | ||
| this.logger.debug('Profile not changed, skipping'); | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| this.logger.debug('Merged profile will be inserted', { | ||
| mergedProfile, | ||
| existingProfile, | ||
| profile, | ||
| }); | ||
| this.logger.debug('Merged profile will be inserted', { | ||
| mergedProfile, | ||
| existingProfile, | ||
| profile, | ||
| }); | ||
|
|
||
| const cacheKey = this.getProfileCacheKey({ | ||
| profileId: profile.id, | ||
| projectId: profile.project_id, | ||
| }); | ||
| const cacheKey = this.getProfileCacheKey({ | ||
| profileId: profile.id, | ||
| projectId: profile.project_id, | ||
| }); | ||
|
|
||
| const result = await this.redis | ||
| .multi() | ||
| .set(cacheKey, JSON.stringify(mergedProfile), 'EX', this.ttlInSeconds) | ||
| .rpush(this.redisKey, JSON.stringify(mergedProfile)) | ||
| .incr(this.bufferCounterKey) | ||
| .llen(this.redisKey) | ||
| .exec(); | ||
| const result = await this.redis | ||
| .multi() | ||
| .set(cacheKey, JSON.stringify(mergedProfile), 'EX', this.ttlInSeconds) | ||
| .rpush(this.redisKey, JSON.stringify(mergedProfile)) | ||
| .incr(this.bufferCounterKey) | ||
| .llen(this.redisKey) | ||
| .exec(); | ||
|
|
||
| if (!result) { | ||
| this.logger.error('Failed to add profile to Redis', { | ||
| profile, | ||
| cacheKey, | ||
| }); | ||
| return; | ||
| } | ||
| const bufferLength = (result?.[3]?.[1] as number) ?? 0; | ||
| if (!result) { | ||
| this.logger.error('Failed to add profile to Redis', { | ||
| profile, | ||
| cacheKey, | ||
| }); | ||
| return; | ||
| } | ||
| const bufferLength = (result?.[3]?.[1] as number) ?? 0; | ||
|
|
||
| this.logger.debug('Current buffer length', { | ||
| bufferLength, | ||
| batchSize: this.batchSize, | ||
| this.logger.debug('Current buffer length', { | ||
| bufferLength, | ||
| batchSize: this.batchSize, | ||
| }); | ||
| if (bufferLength >= this.batchSize) { | ||
| await this.tryFlush(); | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
Release the per-profile lock before tryFlush().
Lines 116-203 currently hold the lock acquired at Line 72 all the way through Line 201. tryFlush() can block on ClickHouse I/O much longer than the 5-second TTL, so the lock can expire mid-critical-section and let a second writer enter with stale state. Capture the post-write buffer length inside the lock, then flush after the callback returns.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/db/src/buffers/profile-buffer.ts` around lines 116 - 203, The
per-profile lock held by withProfileLock (wrapper used around the write code)
currently remains held across the call to tryFlush(), which can block and allow
the lock TTL to expire; fix by moving the tryFlush() call outside the
withProfileLock callback: inside the callback (in the function passed to
withProfileLock) perform the fetch/merge, the Redis multi().exec(), capture the
resulting bufferLength (from result[3][1]) and any relevant cacheKey/profile
info, then return that bufferLength (or set a local variable). After the
withProfileLock completes (lock released), check the captured bufferLength
against this.batchSize and call this.tryFlush() only if >= batchSize. Ensure the
function names referenced are withProfileLock, tryFlush, getProfileCacheKey, and
the redis multi/exec handling that computes bufferLength.
Comment on lines
+143
to
+152
| const mergedProfile: IClickhouseProfile = existingProfile | ||
| ? { | ||
| ...deepMergeObjects( | ||
| existingProfile, | ||
| omit(['created_at', 'groups'], profile) | ||
| ), | ||
| groups: uniq([ | ||
| ...(existingProfile.groups ?? []), | ||
| ...(profile.groups ?? []), | ||
| ]), |
Contributor
There was a problem hiding this comment.
Include groups in the ClickHouse backfill query.
Lines 149-152 merge existingProfile.groups, but the fallback query never selects that column. On a cache miss, existingProfile.groups becomes undefined, so a later identify/update can overwrite previously stored group memberships with an empty array.
Proposed fix
`SELECT
id,
project_id,
last_value(nullIf(first_name, '')) as first_name,
last_value(nullIf(last_name, '')) as last_name,
last_value(nullIf(email, '')) as email,
last_value(nullIf(avatar, '')) as avatar,
last_value(is_external) as is_external,
last_value(properties) as properties,
+ last_value(groups) as groups,
last_value(created_at) as created_at
FROM ${TABLE_NAMES.profiles} Also applies to: 240-267
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/db/src/buffers/profile-buffer.ts` around lines 143 - 152, The
backfill/select query that populates existingProfile is not selecting the groups
column, causing mergedProfile.groups to fall back to undefined and later
overwrite stored groups; update the ClickHouse backfill/select query used when
loading existingProfile (the query that feeds into the code which constructs
mergedProfile in profile-buffer.ts) to include the groups column so
existingProfile.groups is populated, and apply the same fix to the other
occurrence referenced around the 240-267 block so both merges preserve existing
group memberships.
Comment on lines
+178
to
+180
| { name: 'purchase', props: { order_id: `ord_${Date.now()}`, revenue: product.price, product_id: product.id, product_name: product.name, quantity: 1 }, revenue: true }, | ||
| { name: 'screen_view', props: { path: `${ORIGIN}/checkout/success`, title: 'Order Confirmed' } }, | ||
| { name: 'checkout_success', props: { order_id: `ord_${Date.now()}`, revenue: product.price } }, |
Contributor
There was a problem hiding this comment.
Reuse one order_id per checkout flow.
These flows call Date.now() separately for related events, so purchase and checkout_success can end up with different order IDs for the same simulated order. That breaks downstream joins and makes the seed data less realistic.
Suggested pattern
- (product) => [
+ (product) => {
+ const orderId = `ord_${Date.now()}`;
+ return [
...
- { name: 'purchase', props: { order_id: `ord_${Date.now()}`, ... }, revenue: true },
- { name: 'checkout_success', props: { order_id: `ord_${Date.now()}`, ... } },
- ],
+ { name: 'purchase', props: { order_id: orderId, ... }, revenue: true },
+ { name: 'checkout_success', props: { order_id: orderId, ... } },
+ ];
+ },Also applies to: 230-231, 267-268, 287-288, 341-342, 377-378
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/seed-events.mjs` around lines 178 - 180, The purchase and
checkout_success event objects are each calling Date.now() separately, producing
mismatched order IDs; fix by generating a single orderId variable (e.g., const
orderId = `ord_${Date.now()}`) for that simulated checkout flow and reuse
orderId in both the purchase and checkout_success props (and any other related
events like those groups where 'purchase' and 'checkout_success' are created);
update all similar blocks (the repeated checkout event groups) to follow this
pattern so all events in a flow share the same order_id.
Comment on lines
+453
to
+457
| if (!res.ok) { | ||
| const text = await res.text().catch(() => ''); | ||
| console.warn(` [WARN] ${res.status} ${payload.type}/${payload.payload?.name ?? ''}: ${text.slice(0, 120)}`); | ||
| } | ||
| return res; |
Contributor
There was a problem hiding this comment.
Treat non-2xx responses as failures.
sendEvent() only warns here, so the callers still count the request as successful. That makes Events sent and Errors inaccurate and can hide a completely broken seed run.
Proposed fix
if (!res.ok) {
const text = await res.text().catch(() => '');
- console.warn(` [WARN] ${res.status} ${payload.type}/${payload.payload?.name ?? ''}: ${text.slice(0, 120)}`);
+ throw new Error(
+ `${res.status} ${payload.type}/${payload.payload?.name ?? ''}: ${text.slice(0, 120)}`
+ );
}
return res;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/seed-events.mjs` around lines 453 - 457, sendEvent() currently only
logs non-2xx responses and still returns the Response, causing callers to treat
failures as successes; change sendEvent (the function that contains the if
(!res.ok) block) to treat non-OK responses as errors by rejecting/throwing:
after reading the response text (const text = await res.text().catch(() => '')),
throw a new Error that includes res.status, payload.type/payload.payload?.name,
and a snippet of the text so callers see the failure and increment error counts
instead of counting it as success. Ensure the function no longer returns a
successful Response on non-OK status.
Comment on lines
+486
to
+499
| // Sessions are spread across the timeline | ||
| const sessionStartOffset = (sessionIndex / totalSessions) * timelineMs; | ||
| const sessionStart = now - timelineMs + sessionStartOffset; | ||
|
|
||
| // Events within session: spread over 2-10 minutes | ||
| const sessionDurationMs = randInt(2, 10) * 60 * 1000; | ||
| const eventCount = session.events.length; | ||
|
|
||
| return session.events.map((event, i) => { | ||
| const eventOffset = eventCount > 1 ? (i / (eventCount - 1)) * sessionDurationMs : 0; | ||
| return { | ||
| ...event, | ||
| timestamp: Math.round(sessionStart + eventOffset), | ||
| }; |
Contributor
There was a problem hiding this comment.
Keep seeded timestamps inside the requested timeline window.
sessionStart is spread across the full timeline, then each session adds another 2-10 minutes of event offsets. The last sessions can therefore end up with future timestamps, which will skew or even invalidate the seeded analytics data.
Proposed fix
- // Sessions are spread across the timeline
- const sessionStartOffset = (sessionIndex / totalSessions) * timelineMs;
- const sessionStart = now - timelineMs + sessionStartOffset;
-
// Events within session: spread over 2-10 minutes
const sessionDurationMs = randInt(2, 10) * 60 * 1000;
const eventCount = session.events.length;
+ const availableWindow = Math.max(timelineMs - sessionDurationMs, 0);
+
+ // Spread session starts only across the portion of the timeline that still
+ // leaves room for the full session duration.
+ const sessionStartOffset =
+ totalSessions > 1 ? (sessionIndex / (totalSessions - 1)) * availableWindow : 0;
+ const sessionStart = now - timelineMs + sessionStartOffset;📝 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
| // Sessions are spread across the timeline | |
| const sessionStartOffset = (sessionIndex / totalSessions) * timelineMs; | |
| const sessionStart = now - timelineMs + sessionStartOffset; | |
| // Events within session: spread over 2-10 minutes | |
| const sessionDurationMs = randInt(2, 10) * 60 * 1000; | |
| const eventCount = session.events.length; | |
| return session.events.map((event, i) => { | |
| const eventOffset = eventCount > 1 ? (i / (eventCount - 1)) * sessionDurationMs : 0; | |
| return { | |
| ...event, | |
| timestamp: Math.round(sessionStart + eventOffset), | |
| }; | |
| // Events within session: spread over 2-10 minutes | |
| const sessionDurationMs = randInt(2, 10) * 60 * 1000; | |
| const eventCount = session.events.length; | |
| const availableWindow = Math.max(timelineMs - sessionDurationMs, 0); | |
| // Spread session starts only across the portion of the timeline that still | |
| // leaves room for the full session duration. | |
| const sessionStartOffset = | |
| totalSessions > 1 ? (sessionIndex / (totalSessions - 1)) * availableWindow : 0; | |
| const sessionStart = now - timelineMs + sessionStartOffset; | |
| return session.events.map((event, i) => { | |
| const eventOffset = eventCount > 1 ? (i / (eventCount - 1)) * sessionDurationMs : 0; | |
| return { | |
| ...event, | |
| timestamp: Math.round(sessionStart + eventOffset), | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/seed-events.mjs` around lines 486 - 499, The seeded event timestamps
can exceed "now" because sessionStart is computed across the timeline and then
events add sessionDurationMs, so ensure sessions end inside the requested window
by subtracting the session duration when computing start: compute
sessionDurationMs first, then set sessionStart = now - timelineMs +
sessionStartOffset - sessionDurationMs (or clamp sessionStart so sessionStart +
sessionDurationMs <= now); update references to sessionStartOffset,
sessionStart, sessionDurationMs and eventOffset accordingly so the last event
timestamp never exceeds now.
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
New Features
Enhancements
Documentation