Pin Drop dashboard customisations - see CHANGES.md#338
Closed
andy-a wants to merge 1 commit intoOpenpanel-dev:mainfrom
Closed
Pin Drop dashboard customisations - see CHANGES.md#338andy-a wants to merge 1 commit intoOpenpanel-dev:mainfrom
andy-a wants to merge 1 commit intoOpenpanel-dev:mainfrom
Conversation
|
|
Contributor
📝 WalkthroughWalkthroughThis PR introduces comprehensive dashboard, API, and development tooling updates to Pin Drop OpenPanel. New profile and group widgets (source, platforms, top members), refactored profile properties display, sortable columns, enriched backend services with metrics, supporting utilities for source classification and date formatting, and seed/deployment documentation are integrated across the frontend, backend, and tooling layers. Changes
Sequence DiagramsequenceDiagram
participant Client as Browser/Client
participant Router as TRPC Router
participant Service as Profile/Group Service
participant DB as ClickHouse Database
Note over Client,DB: Enriched Profile/Group Data Flow (New Features)
Client->>Router: profile.list (with sortBy, sortDirection)
Router->>Service: getEnrichedProfileList(input)
Service->>DB: Query profiles FINAL
Service->>DB: Query session aggregates (eventCount, totalDuration, lastSeen)
DB-->>Service: Return metrics per profile
Service->>Service: Join & compute enrichments (plan, isSubscriber)
Service-->>Router: Return IEnrichedServiceProfile[]
Router-->>Client: Send enriched profile data
Client->>Client: Render SortableHeader & columns<br/>(eventCount, totalDuration, plan)
Note over Client,DB: Profile Source & Platforms Discovery
Client->>Router: profile.source (profileId, projectId)
Router->>DB: Query earliest session + referrer/UTM rollup
DB-->>Router: Return acquisition data
Router-->>Client: Return ClassifiedSource[]
Client->>Router: profile.platforms (profileId, projectId)
Router->>DB: Aggregate events by sdk_name (sessions, browsers, version)
DB-->>Router: Return platform metrics
Router-->>Client: Return platform distribution + stats
Client->>Client: Render ProfileSource widget<br/>(ChannelIcon, CHANNEL_BADGE_CLASS)
Client->>Client: Render ProfilePlatforms widget<br/>(segmented bar, session %)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (18)
apps/start/src/routes/_app.$organizationId.$projectId.groups_.$groupId._tabs.tsx (1)
104-110: Add an explicit breadcrumb label for accessibility.Consider labeling the breadcrumb nav so screen readers can identify its purpose immediately.
Suggested tweak
- <nav className="mb-2 flex items-center gap-1 text-sm text-muted-foreground"> + <nav + aria-label="Breadcrumb" + className="mb-2 flex items-center gap-1 text-sm text-muted-foreground" + >🤖 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.tsx around lines 104 - 110, The breadcrumb nav lacks an explicit accessible label; update the nav element (the <nav className="mb-2 flex items-center gap-1 text-sm text-muted-foreground"> that wraps ProjectLink, ChevronRight and {g.name}) to include an ARIA label or aria-labelledby (e.g., aria-label="Breadcrumb" or aria-label="Breadcrumb navigation") so screen readers can identify it as a breadcrumb region; ensure the label is concise and unique and keep the inner structure (ProjectLink, ChevronRight, span with {g.name}) unchanged.apps/start/src/components/ui/data-table/data-table-hooks.tsx (1)
35-43: Consider memoizingsetSortwithuseCallback.The
setSortfunction is recreated on every render. If consuming components pass it to memoized children or include it in dependency arrays, this could cause unnecessary re-renders.♻️ Proposed optimization
+import { useCallback, useEffect, useState } from 'react'; -import { useEffect, useState } from 'react'; ... + const setSort = useCallback( + (next: { id: string; desc: boolean } | null) => { + if (!next) { + setSortBy(''); + setDirection(defaultDirection); + return; + } + setSortBy(next.id); + setDirection(next.desc ? 'desc' : 'asc'); + }, + [setSortBy, setDirection, defaultDirection], + ); return { sortBy: effectiveSortBy, sortDirection: direction, - setSort: (next: { id: string; desc: boolean } | null) => { - if (!next) { - setSortBy(''); - setDirection(defaultDirection); - return; - } - setSortBy(next.id); - setDirection(next.desc ? 'desc' : 'asc'); - }, + setSort, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/start/src/components/ui/data-table/data-table-hooks.tsx` around lines 35 - 43, Wrap the setSort callback in React.useCallback to avoid recreating it on every render: replace the inline setSort definition with a useCallback hook that returns the same logic and list its dependencies (setSortBy, setDirection, defaultDirection) so the memoized function updates correctly; also ensure useCallback is imported from React if not already.apps/start/src/routes/_app.$organizationId.$projectId.profiles._tabs.anonymous.tsx (1)
41-42: Avoidas anytype assertion forsortBy.The
as anycast bypasses type safety. IfuseDataTableSortreturned a properly typed value matching the tRPCzSortByschema, the cast wouldn't be needed. Consider typing the hook's return value to match the expected enum, or use a type guard/assertion function.♻️ Suggested approach
If
ProfileListSortByis the expected type from the tRPC schema, consider updating the hook or adding a type-safe conversion:// Option 1: Type the hook parameter const { sortBy, sortDirection } = useDataTableSort<ProfileListSortBy>('createdAt', 'desc'); // Option 2: Use a type-safe assertion sortBy: sortBy as ProfileListSortBy | undefined,🤖 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.profiles._tabs.anonymous.tsx around lines 41 - 42, The current use of "sortBy: (sortBy ?? undefined) as any" bypasses type safety; update the type so the value matches the tRPC zSortBy/ ProfileListSortBy enum instead of casting to any. Fix by either typing the hook return (e.g., make useDataTableSort generic or return ProfileListSortBy | undefined so the destructured sortBy is already correctly typed) or replace the cast with a narrow assertion such as sortBy as ProfileListSortBy | undefined, or implement a small type-guard/assertion function that validates sortBy against ProfileListSortBy before passing it to the query; reference symbols: sortBy, useDataTableSort, zSortBy, ProfileListSortBy.CHANGES.md (1)
223-224: Minor terminology update: "Bing Ads" is now "Microsoft Ads".Microsoft rebranded Bing Ads to Microsoft Advertising (commonly referred to as "Microsoft Ads"). Consider updating for accuracy.
📝 Suggested fix
- Reddit, X/Twitter Ads, Pinterest, Snapchat, Microsoft (Bing Ads), + Reddit, X/Twitter Ads, Pinterest, Snapchat, Microsoft Ads,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGES.md` around lines 223 - 224, Update the terminology fragment "Microsoft (Bing Ads)" to use the current branding; replace that exact text with "Microsoft Ads" (or "Microsoft Advertising (Microsoft Ads)" if you want the full name) so the CHANGES entry reads e.g. "Reddit, X/Twitter Ads, Pinterest, Snapchat, Microsoft Ads, YouTube."LOCAL_SETUP.md (2)
30-32: Replace hardcoded path with a generic placeholder.The path
/Users/andy/Websites/openpanelis user-specific. Consider using a generic instruction:-cd /Users/andy/Websites/openpanel +cd <path-to-your-openpanel-clone>Or simply:
cd openpanel # wherever you cloned the repoThis makes the doc reusable if others need it later.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@LOCAL_SETUP.md` around lines 30 - 32, Replace the hardcoded user-specific path "cd /Users/andy/Websites/openpanel" in the LOCAL_SETUP.md bash example with a generic placeholder such as "cd openpanel # wherever you cloned the repo" (or instruct users to replace with their own path) so the doc is reusable across machines; update the code block that contains the "cd /Users/andy/Websites/openpanel" line accordingly.
1-5: Consider generalizing the document title.The title "for Andy" limits the document's reusability. If this is intended to be a general contributor guide, consider renaming to just "OpenPanel — Local Development Setup".
If it's intentionally personalized for a specific handoff, this is fine to leave as-is.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@LOCAL_SETUP.md` around lines 1 - 5, The document title "OpenPanel — Local Development Setup (for Andy)" is overly personalized; update the top-level header text to "OpenPanel — Local Development Setup" to generalize it for all contributors (replace the exact string "OpenPanel — Local Development Setup (for Andy)" in the file). If the personalization was intentional, add a short note below the header clarifying the audience instead of embedding a name in the title.packages/db/src/services/group.service.ts (1)
272-293: Consider using the custom query builder for ClickHouse queries.The refactored queries use raw SQL strings with
chQuery. As per coding guidelines, ClickHouse queries should use the custom query builder from./packages/db/src/clickhouse/query-builder.tsand./packages/db/src/clickhouse/query-functions.ts.That said, the existing codebase in this file already uses raw SQL throughout, so this is consistent with the current pattern. If the team wants to migrate to the query builder, it could be done in a follow-up.
The logic itself is correct — counting members from the
profilestable withARRAY JOIN groupsensures consistency with the Members tab display.🤖 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 272 - 293, The two raw ClickHouse calls using chQuery that populate memberRows and activityRows (the SELECTs against TABLE_NAMES.profiles and TABLE_NAMES.events using ARRAY JOIN groups and escapedIds/projectId) should be rewritten to use the project's ClickHouse query builder and query-functions instead of raw SQL strings; replace the chQuery<{...}>(`...`) invocations that build the member count and last_active_at queries with the corresponding builder API from clickhouse/query-builder.ts and helper functions from clickhouse/query-functions.ts, passing projectId and the escaped group IDs via the builder methods so parameters are safely composed and the queries follow the shared query-building conventions.apps/start/src/components/profiles/profile-source.tsx (2)
23-39: Consider using a lookup object instead of nested ternaries.The deeply nested ternary is hard to read and maintain. A lookup object would be cleaner:
♻️ Suggested refactor
+const CHANNEL_ICONS: Record<SourceChannel, typeof Globe> = { + 'paid-search': Megaphone, + 'paid-social': Megaphone, + 'paid-video': Video, + 'organic-search': Search, + email: Mail, + 'organic-social': Share2, + referral: Globe, + direct: LinkIcon, +}; + function ChannelIcon({ channel }: { channel: SourceChannel }) { - const Icon = - channel === 'paid-search' || channel === 'paid-social' - ? Megaphone - : channel === 'paid-video' - ? Video - : ... // truncated + const Icon = CHANNEL_ICONS[channel] ?? LinkIcon; return <Icon className="size-4 shrink-0 text-muted-foreground" />; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/start/src/components/profiles/profile-source.tsx` around lines 23 - 39, Replace the nested ternary in ChannelIcon with a clear lookup map: create an object mapping SourceChannel values ('paid-search','paid-social','paid-video','organic-search','email','organic-social','referral') to the corresponding icon symbols (Megaphone, Video, Search, Mail, Share2, Globe) and use LinkIcon as the default fallback (e.g., const ICON_MAP = { ... }; const Icon = ICON_MAP[channel] || LinkIcon;), then return <Icon ... /> unchanged; this keeps the ChannelIcon component and prop typing intact while improving readability and maintainability.
52-76: Same refactor opportunity forChannelBadge.Similarly, the readable label mapping could use a lookup:
♻️ Suggested refactor
+const CHANNEL_LABELS: Record<SourceChannel, string> = { + 'paid-search': 'Paid search', + 'paid-social': 'Paid social', + 'paid-video': 'Paid video', + 'organic-search': 'Organic search', + 'organic-social': 'Organic social', + email: 'Email', + referral: 'Referral', + direct: 'Direct', +}; + function ChannelBadge({ channel }: { channel: SourceChannel }) { - const readable = - channel === 'paid-search' - ? 'Paid search' - : ... // truncated + const readable = CHANNEL_LABELS[channel] ?? 'Unknown'; return ( <span className={...}> {readable} </span> ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/start/src/components/profiles/profile-source.tsx` around lines 52 - 76, The ChannelBadge component currently uses a long nested ternary to derive a human-readable label; replace that with a lookup object (e.g., const CHANNEL_LABELS: Record<SourceChannel, string>) and use CHANNEL_LABELS[channel] (with a fallback like 'Direct') to compute readable; update ChannelBadge to reference the new CHANNEL_LABELS mapping and keep the existing CHANNEL_BADGE_CLASS[channel] usage to preserve styling.apps/start/src/utils/date.ts (1)
67-87: Consider handling "days" and "weeks" for consistency.The function normalizes most time units (
seconds,minutes,hours,months,years) but doesn't handledaysorweeks. Depending on your UI constraints, you might want to add:.replace(/\byears?\b/g, 'yrs') + .replace(/\bweeks?\b/g, 'wks') + .replace(/\bdays?\b/g, 'days') // or 'd' if you want it shorterThis is minor since "days" and "weeks" are already fairly short, but flagging for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/start/src/utils/date.ts` around lines 67 - 87, The timeAgoShort function (using ta.format) currently normalizes seconds/minutes/hours/months/years but omits days and weeks; update timeAgoShort to also replace full words "days" and "weeks" with short forms (e.g., "days" -> "dys" or "days" -> "days" depending on your chosen short form) and handle any locale-specific abbreviated forms with trailing periods (e.g., "day." / "wk.") and add the singular-case replacement analogous to the existing .replace(/\b1 (secs|mins|hrs|mos|yrs)\b/g, ...) to drop the plural "s" for "1 day" and "1 week"; make changes in the timeAgoShort function around the other .replace calls so days/weeks follow the same pattern and naming conventions as the other units.apps/start/src/components/groups/group-platforms.tsx (1)
57-67: Minor: Potential duplicate keys in the bar chart.If two different SDKs map to the same
label(e.g., bothswiftandiosSDK names map to "iOS"), the keybar-${p.label}would be duplicated, causing a React warning.Consider using a unique identifier:
-key={`bar-${p.label}`} +key={`bar-${p.sdkName ?? p.label}`}This is unlikely to occur in practice, but worth noting for robustness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/start/src/components/groups/group-platforms.tsx` around lines 57 - 67, The bar elements use a non-unique key `bar-${p.label}` in the data.map callback which can duplicate if different SDK entries share the same label; update the key to be unique by combining the label with another stable identifier (e.g., `p.id`, `p.key`, or the map index) used by the data source so the key becomes something like `bar-${p.label}-${p.id}` or `bar-${p.label}-${index}`; locate the map over `data` in group-platforms.tsx and replace the `key` prop on the returned div accordingly.apps/start/src/utils/source.ts (1)
195-203: Operator precedence may cause unexpected behavior.The condition
referrerType === 'social' || referrerName && /social/i.test(referrerName)relies on JavaScript's operator precedence where&&binds tighter than||. While this works correctly, adding parentheses improves readability and prevents accidental bugs if the condition is modified later.♻️ Suggested clarification
- if (referrerType === 'social' || referrerName && /social/i.test(referrerName)) { + if (referrerType === 'social' || (referrerName && /social/i.test(referrerName))) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/start/src/utils/source.ts` around lines 195 - 203, The boolean expression using referrerType and referrerName relies on && binding tighter than ||; update the conditional in the block that builds the organic-social object (the if using referrerType === 'social' || referrerName && /social/i.test(referrerName)) to add explicit parentheses so the intent is clear (e.g. referrerType === 'social' || (referrerName && /social/i.test(referrerName))); no other logic changes required—just group the second operand to improve readability and prevent future precedence mistakes.packages/trpc/src/routers/group.ts (1)
102-118: Consider performance implications of correlated subquery for large groups.The
profile_id IN (SELECT ...)pattern requires ClickHouse to materialize the inner subquery for each row evaluation. For groups with many members, this could be slow compared to a JOIN-based approach.If performance becomes an issue, consider rewriting as a JOIN:
♻️ Alternative using JOIN
- chQuery<{ totalSessions: number; totalSessionDuration: number }>(` - SELECT - count() AS totalSessions, - -- minutes - round(sum(duration) / 60, 2) AS totalSessionDuration - FROM ${TABLE_NAMES.sessions} FINAL - WHERE project_id = ${sqlstring.escape(projectId)} - AND profile_id IN ( - SELECT id FROM ${TABLE_NAMES.profiles} FINAL - WHERE project_id = ${sqlstring.escape(projectId)} - AND has(groups, ${sqlstring.escape(id)}) - ) - `), + chQuery<{ totalSessions: number; totalSessionDuration: number }>(` + SELECT + count() AS totalSessions, + round(sum(s.duration) / 60, 2) AS totalSessionDuration + FROM ${TABLE_NAMES.sessions} AS s FINAL + INNER JOIN ( + SELECT id FROM ${TABLE_NAMES.profiles} FINAL + WHERE project_id = ${sqlstring.escape(projectId)} + AND has(groups, ${sqlstring.escape(id)}) + ) AS p ON s.profile_id = p.id + WHERE s.project_id = ${sqlstring.escape(projectId)} + `),🤖 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 102 - 118, The current chQuery in group router uses a correlated subquery (profile_id IN (SELECT id FROM ${TABLE_NAMES.profiles} ...)) which can be slow for large groups; replace that IN-subquery with an explicit JOIN between ${TABLE_NAMES.sessions} and ${TABLE_NAMES.profiles} (e.g., alias sessions and profiles) filtering on project_id and has(groups, ${sqlstring.escape(id)}) to compute totalSessions and totalSessionDuration, ensuring you still use the same projectId and id variables and keep the FINAL table modifier and aggregation logic in the chQuery call in packages/trpc/src/routers/group.ts.apps/start/src/components/profiles/table/columns.tsx (1)
138-143: Minor inconsistency: eventCount shows0while other columns show—for missing values.The eventCount column displays
0for missing values (via?? 0), while totalDuration, firstSeenActivity, and lastSeen show—. Consider whether0is the intended display for profiles with no events.If
0is intentional (a profile with no events truly has 0 events), this is fine. If it should match the dash pattern:♻️ Optional: match dash pattern
cell: ({ row }) => ( <div className="text-right font-mono tabular-nums"> - {row.original.eventCount?.toLocaleString() ?? 0} + {row.original.eventCount ? row.original.eventCount.toLocaleString() : '—'} </div> ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/start/src/components/profiles/table/columns.tsx` around lines 138 - 143, Change the eventCount cell to use the same missing-value dash as other columns instead of defaulting to 0: in the cell renderer (cell: ({ row }) => ...), check for null/undefined on row.original.eventCount and render row.original.eventCount.toLocaleString() only when present, otherwise render '—' (i.e., replace the current `?? 0` fallback logic with an explicit null check and the dash fallback).apps/start/src/routes/_app.$organizationId.$projectId.groups_.$groupId._tabs.index.tsx (1)
17-26: Consider usingIntl.DateTimeFormatfor locale-aware date formatting.The manual DD/MM/YYYY format works, but
Intl.DateTimeFormatwould be more maintainable and could respect user locale if needed in the future.♻️ Optional refactor
function formatDateShort(value: string | Date | null | undefined): string { if (!value) return '—'; const d = value instanceof Date ? value : new Date(value); if (Number.isNaN(d.getTime())) return '—'; - const dd = String(d.getDate()).padStart(2, '0'); - const mm = String(d.getMonth() + 1).padStart(2, '0'); - const yyyy = d.getFullYear(); - return `${dd}/${mm}/${yyyy}`; + return new Intl.DateTimeFormat('en-GB', { + day: '2-digit', + month: '2-digit', + year: 'numeric', + }).format(d); }🤖 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 17 - 26, The formatDateShort function manually constructs DD/MM/YYYY strings; replace that logic with Intl.DateTimeFormat to produce the same DD/MM/YYYY output while keeping the existing null/invalid handling: in formatDateShort, convert value to a Date, return '—' for falsy/invalid dates, then format with new Intl.DateTimeFormat('en-GB', { day: '2-digit', month: '2-digit', year: 'numeric' }) (or use a locale variable if you want locale-awareness later) to generate the final string; keep the function signature and return behavior unchanged.packages/trpc/src/routers/profile.ts (3)
69-92: Consider usingargMaxfor consistent OS selection.Line 71 uses
any(os)which picks an arbitrary OS value from the group. Since you're already usingargMaxforapp_versionandbuild_numberto get the most recent values, consider using the same approach forosto ensure consistency.♻️ Suggested change
SELECT sdk_name, - any(os) as os, + argMax(os, created_at) as os,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/trpc/src/routers/profile.ts` around lines 69 - 92, The query in profile.ts uses any(os) which can return an arbitrary OS; replace it with argMax(os, created_at) to pick the OS from the most recent event for each sdk_name so it matches how app_version and build_number are selected; update the SELECT to use argMax(os, created_at) (keeping created_at already used elsewhere) and ensure grouping/ordering remain the same for functions like groupUniqArray, argMax(properties['__version'], created_at), and argMax(properties['__buildNumber'], created_at).
121-121: Inconsistent date return types across procedures.The new
platformsandsourceprocedures return dates as raw strings (e.g.,lastSeen: r.last_seen,createdAt: first.created_at), whilegetEnrichedProfileListused bylistandpowerUsersreturnsDateobjects viaconvertClickhouseDateToJs. This may cause inconsistent handling in the UI layer.Consider normalizing to return
Dateobjects for consistency with the rest of the profile router.Also applies to: 194-194
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/trpc/src/routers/profile.ts` at line 121, The platforms and source procedures are returning raw date strings (e.g., mapping lastSeen: r.last_seen and createdAt: first.created_at) while getEnrichedProfileList returns Date objects; update the platforms and source result mappings to convert ClickHouse date strings to JS Dates using the existing convertClickhouseDateToJs helper so lastSeen and createdAt (and any other *_at / last_* fields) return Date objects like getEnrichedProfileList; locate the mappings inside the platforms and source procedure handlers and replace direct field assignments with calls to convertClickhouseDateToJs(r.last_seen) and convertClickhouseDateToJs(first.created_at) (and similar fields mentioned at lines 121 and 194).
56-93: Raw SQL query instead of custom query builder.The coding guidelines specify using the custom query builder from
./packages/db/src/clickhouse/query-builder.tsfor ClickHouse queries. However, I note that existing procedures in this file (activity,mostEvents,popularRoutes) also use raw SQL withchQuery. If this is an intentional pattern for this router, please confirm; otherwise, consider refactoring to usecreateSqlBuilder.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/trpc/src/routers/profile.ts` around lines 56 - 93, The platforms procedure currently uses a raw SQL string passed to chQuery (see platforms/protectedProcedure and chQuery with TABLE_NAMES.events), which violates the guideline to use the ClickHouse custom query builder; refactor this to use createSqlBuilder from ./packages/db/src/clickhouse/query-builder.ts to build the same SELECT (sdk_name, any(os) as os, arrayFilter/... as browsers, count(distinct session_id) as sessions, count() as events, max(created_at) as last_seen, argMax(properties['__version'], created_at) as app_version, argMax(properties['__buildNumber'], created_at) as build_number) against TABLE_NAMES.events, and pass parameters (profileId, projectId) through the builder (not string interpolation) before calling chQuery; update the other procedures in this file (activity, mostEvents, popularRoutes) similarly if they still use raw SQL to ensure consistent, parameterized query construction via createSqlBuilder.
🤖 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/profile-avatar.tsx`:
- Around line 86-91: The current avatar selection uses a prefix-based
isValidAvatar check that can accept malformed or insecure schemes; update the
validation used before prioritizing the explicit avatar so it parses the string
as a URL and only treats it as valid when new URL(avatar) succeeds and
url.protocol === 'https:' (and optionally url.hostname is non-empty), then use
that tightened check in the branch that sets src (where useGravatarUrl,
isValidAvatar, avatar, gravatarUrl and src are referenced) so only fully-formed
HTTPS URLs override Gravatar; if parsing fails or protocol isn't 'https:' fall
back to gravatarUrl.
In `@apps/start/src/components/profiles/profile-platforms.tsx`:
- Around line 77-131: The list rendered in data.map uses p.label as the React
key which can collide; change the key on the <li> to a stable unique identifier
(for example use p.id if available, or build a composite key like
`${p.label}-${p.appVersion ?? 'web'}-${p.buildNumber ?? ''}`) instead of p.label
so each <li> rendered by the data.map call has a truly unique key; update the
key usage in the JSX returned by data.map (the <li key=...> in this component)
accordingly.
- Around line 63-73: The current bar segment key uses `p.label` which can
collide when `friendlyLabel` maps multiple `sdk_name` values to the same label;
update the key in the `data.map` rendering (the map callback where
`key={`bar-${p.label}`}` is set) to include a unique identifier such as
`p.sdkName` or a combination like `bar-${p.sdkName}-${p.label}` so each segment
is uniquely keyed and avoids React key collisions.
In `@apps/start/src/components/profiles/profile-properties.tsx`:
- Around line 47-62: The component ProfileProperties currently dereferences
profile (e.g., profile.projectId, profile.groups) before any JSX guard so a
null/undefined profile will throw; either make the prop required in the Props
type or add an early return guard at the top of ProfileProperties (before
useTRPC/useQuery usage) that returns the "No profile data" fallback when profile
is nullish; also apply the same early-guard fix for the later usage referenced
around the block that corresponds to lines 167-175 so no further code
dereferences profile after a null check.
- Around line 64-67: The code assigns `team` by falling back to the first group
if no group has type 'team', which mislabels non-team groups; change the `team`
assignment to only pick groupsQuery.data?.find((g) => g.type?.toLowerCase() ===
'team') and otherwise set `team` to null (remove the `groupsQuery.data?.[0]`
fallback), then update any places that render `teamName` (e.g., where you read
`team?.name` around the current `team` usage and the other occurrence at the
later usage) to handle null/undefined (show empty string or a proper
placeholder) instead of implicitly trusting a fallback group.
In `@DEPLOYMENT.md`:
- Around line 154-167: The docker push commands use hardcoded tags (e.g.,
ghcr.io/pindrop/openpanel-dashboard:custom-20260416) that don’t match the build
tags which use $(date +%Y%m%d); fix by generating a single TAG variable or
reusing $(date +%Y%m%d) so build and push use the same tag. Specifically, set
TAG=$(date +%Y%m%d) (or use the same command substitution) and replace the
hardcoded tags in the docker push lines so they match the tags emitted by the
docker build lines.
In `@packages/db/src/services/profile.service.ts`:
- Around line 288-290: The search filter currently interpolates `search`
directly into `filterSearch`, causing SQL injection; change the logic in the
`filterSearch` construction to use a parameterized placeholder and a params
array (e.g., add a parameter like `'%'+search+'%'` or use SQL concatenation with
`ILIKE '%' || $n || '%'`) instead of string interpolation, update the query
execution to pass that params array, and apply the same pattern to the other
occurrences in `getProfileList` and `getProfileListCount` so that `search` is
always bound as a query parameter rather than injected into the SQL string.
- Around line 304-313: The session duration units are inconsistent:
getProfileMetrics returns minutes while getEnrichedProfileList's session_stats
CTE (the SUM on ${TABLE_NAMES.sessions} via sum(duration) AS total_duration)
returns raw milliseconds; update the session_stats aggregation in
getEnrichedProfileList to convert milliseconds to minutes (matching
getProfileMetrics), e.g. divide sum(duration) by 1000 and 60 and round
similarly, so total_duration uses the same minutes unit and precision as
getProfileMetrics.
---
Nitpick comments:
In `@apps/start/src/components/groups/group-platforms.tsx`:
- Around line 57-67: The bar elements use a non-unique key `bar-${p.label}` in
the data.map callback which can duplicate if different SDK entries share the
same label; update the key to be unique by combining the label with another
stable identifier (e.g., `p.id`, `p.key`, or the map index) used by the data
source so the key becomes something like `bar-${p.label}-${p.id}` or
`bar-${p.label}-${index}`; locate the map over `data` in group-platforms.tsx and
replace the `key` prop on the returned div accordingly.
In `@apps/start/src/components/profiles/profile-source.tsx`:
- Around line 23-39: Replace the nested ternary in ChannelIcon with a clear
lookup map: create an object mapping SourceChannel values
('paid-search','paid-social','paid-video','organic-search','email','organic-social','referral')
to the corresponding icon symbols (Megaphone, Video, Search, Mail, Share2,
Globe) and use LinkIcon as the default fallback (e.g., const ICON_MAP = { ... };
const Icon = ICON_MAP[channel] || LinkIcon;), then return <Icon ... />
unchanged; this keeps the ChannelIcon component and prop typing intact while
improving readability and maintainability.
- Around line 52-76: The ChannelBadge component currently uses a long nested
ternary to derive a human-readable label; replace that with a lookup object
(e.g., const CHANNEL_LABELS: Record<SourceChannel, string>) and use
CHANNEL_LABELS[channel] (with a fallback like 'Direct') to compute readable;
update ChannelBadge to reference the new CHANNEL_LABELS mapping and keep the
existing CHANNEL_BADGE_CLASS[channel] usage to preserve styling.
In `@apps/start/src/components/profiles/table/columns.tsx`:
- Around line 138-143: Change the eventCount cell to use the same missing-value
dash as other columns instead of defaulting to 0: in the cell renderer (cell: ({
row }) => ...), check for null/undefined on row.original.eventCount and render
row.original.eventCount.toLocaleString() only when present, otherwise render '—'
(i.e., replace the current `?? 0` fallback logic with an explicit null check and
the dash fallback).
In `@apps/start/src/components/ui/data-table/data-table-hooks.tsx`:
- Around line 35-43: Wrap the setSort callback in React.useCallback to avoid
recreating it on every render: replace the inline setSort definition with a
useCallback hook that returns the same logic and list its dependencies
(setSortBy, setDirection, defaultDirection) so the memoized function updates
correctly; also ensure useCallback is imported from React if not already.
In
`@apps/start/src/routes/_app`.$organizationId.$projectId.groups_.$groupId._tabs.index.tsx:
- Around line 17-26: The formatDateShort function manually constructs DD/MM/YYYY
strings; replace that logic with Intl.DateTimeFormat to produce the same
DD/MM/YYYY output while keeping the existing null/invalid handling: in
formatDateShort, convert value to a Date, return '—' for falsy/invalid dates,
then format with new Intl.DateTimeFormat('en-GB', { day: '2-digit', month:
'2-digit', year: 'numeric' }) (or use a locale variable if you want
locale-awareness later) to generate the final string; keep the function
signature and return behavior unchanged.
In
`@apps/start/src/routes/_app`.$organizationId.$projectId.groups_.$groupId._tabs.tsx:
- Around line 104-110: The breadcrumb nav lacks an explicit accessible label;
update the nav element (the <nav className="mb-2 flex items-center gap-1 text-sm
text-muted-foreground"> that wraps ProjectLink, ChevronRight and {g.name}) to
include an ARIA label or aria-labelledby (e.g., aria-label="Breadcrumb" or
aria-label="Breadcrumb navigation") so screen readers can identify it as a
breadcrumb region; ensure the label is concise and unique and keep the inner
structure (ProjectLink, ChevronRight, span with {g.name}) unchanged.
In
`@apps/start/src/routes/_app`.$organizationId.$projectId.profiles._tabs.anonymous.tsx:
- Around line 41-42: The current use of "sortBy: (sortBy ?? undefined) as any"
bypasses type safety; update the type so the value matches the tRPC zSortBy/
ProfileListSortBy enum instead of casting to any. Fix by either typing the hook
return (e.g., make useDataTableSort generic or return ProfileListSortBy |
undefined so the destructured sortBy is already correctly typed) or replace the
cast with a narrow assertion such as sortBy as ProfileListSortBy | undefined, or
implement a small type-guard/assertion function that validates sortBy against
ProfileListSortBy before passing it to the query; reference symbols: sortBy,
useDataTableSort, zSortBy, ProfileListSortBy.
In `@apps/start/src/utils/date.ts`:
- Around line 67-87: The timeAgoShort function (using ta.format) currently
normalizes seconds/minutes/hours/months/years but omits days and weeks; update
timeAgoShort to also replace full words "days" and "weeks" with short forms
(e.g., "days" -> "dys" or "days" -> "days" depending on your chosen short form)
and handle any locale-specific abbreviated forms with trailing periods (e.g.,
"day." / "wk.") and add the singular-case replacement analogous to the existing
.replace(/\b1 (secs|mins|hrs|mos|yrs)\b/g, ...) to drop the plural "s" for "1
day" and "1 week"; make changes in the timeAgoShort function around the other
.replace calls so days/weeks follow the same pattern and naming conventions as
the other units.
In `@apps/start/src/utils/source.ts`:
- Around line 195-203: The boolean expression using referrerType and
referrerName relies on && binding tighter than ||; update the conditional in the
block that builds the organic-social object (the if using referrerType ===
'social' || referrerName && /social/i.test(referrerName)) to add explicit
parentheses so the intent is clear (e.g. referrerType === 'social' ||
(referrerName && /social/i.test(referrerName))); no other logic changes
required—just group the second operand to improve readability and prevent future
precedence mistakes.
In `@CHANGES.md`:
- Around line 223-224: Update the terminology fragment "Microsoft (Bing Ads)" to
use the current branding; replace that exact text with "Microsoft Ads" (or
"Microsoft Advertising (Microsoft Ads)" if you want the full name) so the
CHANGES entry reads e.g. "Reddit, X/Twitter Ads, Pinterest, Snapchat, Microsoft
Ads, YouTube."
In `@LOCAL_SETUP.md`:
- Around line 30-32: Replace the hardcoded user-specific path "cd
/Users/andy/Websites/openpanel" in the LOCAL_SETUP.md bash example with a
generic placeholder such as "cd openpanel # wherever you cloned the repo" (or
instruct users to replace with their own path) so the doc is reusable across
machines; update the code block that contains the "cd
/Users/andy/Websites/openpanel" line accordingly.
- Around line 1-5: The document title "OpenPanel — Local Development Setup (for
Andy)" is overly personalized; update the top-level header text to "OpenPanel —
Local Development Setup" to generalize it for all contributors (replace the
exact string "OpenPanel — Local Development Setup (for Andy)" in the file). If
the personalization was intentional, add a short note below the header
clarifying the audience instead of embedding a name in the title.
In `@packages/db/src/services/group.service.ts`:
- Around line 272-293: The two raw ClickHouse calls using chQuery that populate
memberRows and activityRows (the SELECTs against TABLE_NAMES.profiles and
TABLE_NAMES.events using ARRAY JOIN groups and escapedIds/projectId) should be
rewritten to use the project's ClickHouse query builder and query-functions
instead of raw SQL strings; replace the chQuery<{...}>(`...`) invocations that
build the member count and last_active_at queries with the corresponding builder
API from clickhouse/query-builder.ts and helper functions from
clickhouse/query-functions.ts, passing projectId and the escaped group IDs via
the builder methods so parameters are safely composed and the queries follow the
shared query-building conventions.
In `@packages/trpc/src/routers/group.ts`:
- Around line 102-118: The current chQuery in group router uses a correlated
subquery (profile_id IN (SELECT id FROM ${TABLE_NAMES.profiles} ...)) which can
be slow for large groups; replace that IN-subquery with an explicit JOIN between
${TABLE_NAMES.sessions} and ${TABLE_NAMES.profiles} (e.g., alias sessions and
profiles) filtering on project_id and has(groups, ${sqlstring.escape(id)}) to
compute totalSessions and totalSessionDuration, ensuring you still use the same
projectId and id variables and keep the FINAL table modifier and aggregation
logic in the chQuery call in packages/trpc/src/routers/group.ts.
In `@packages/trpc/src/routers/profile.ts`:
- Around line 69-92: The query in profile.ts uses any(os) which can return an
arbitrary OS; replace it with argMax(os, created_at) to pick the OS from the
most recent event for each sdk_name so it matches how app_version and
build_number are selected; update the SELECT to use argMax(os, created_at)
(keeping created_at already used elsewhere) and ensure grouping/ordering remain
the same for functions like groupUniqArray, argMax(properties['__version'],
created_at), and argMax(properties['__buildNumber'], created_at).
- Line 121: The platforms and source procedures are returning raw date strings
(e.g., mapping lastSeen: r.last_seen and createdAt: first.created_at) while
getEnrichedProfileList returns Date objects; update the platforms and source
result mappings to convert ClickHouse date strings to JS Dates using the
existing convertClickhouseDateToJs helper so lastSeen and createdAt (and any
other *_at / last_* fields) return Date objects like getEnrichedProfileList;
locate the mappings inside the platforms and source procedure handlers and
replace direct field assignments with calls to
convertClickhouseDateToJs(r.last_seen) and
convertClickhouseDateToJs(first.created_at) (and similar fields mentioned at
lines 121 and 194).
- Around line 56-93: The platforms procedure currently uses a raw SQL string
passed to chQuery (see platforms/protectedProcedure and chQuery with
TABLE_NAMES.events), which violates the guideline to use the ClickHouse custom
query builder; refactor this to use createSqlBuilder from
./packages/db/src/clickhouse/query-builder.ts to build the same SELECT
(sdk_name, any(os) as os, arrayFilter/... as browsers, count(distinct
session_id) as sessions, count() as events, max(created_at) as last_seen,
argMax(properties['__version'], created_at) as app_version,
argMax(properties['__buildNumber'], created_at) as build_number) against
TABLE_NAMES.events, and pass parameters (profileId, projectId) through the
builder (not string interpolation) before calling chQuery; update the other
procedures in this file (activity, mostEvents, popularRoutes) similarly if they
still use raw SQL to ensure consistent, parameterized query construction via
createSqlBuilder.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6324d246-4dbf-488f-8fde-795dfcd94eb9
📒 Files selected for processing (37)
CHANGES.mdDEPLOYMENT.mdLOCAL_SETUP.mdapps/start/src/components/events/event-list-item.tsxapps/start/src/components/groups/group-platforms.tsxapps/start/src/components/groups/group-top-members.tsxapps/start/src/components/groups/table/columns.tsxapps/start/src/components/overview/overview-metric-card.tsxapps/start/src/components/profiles/latest-events.tsxapps/start/src/components/profiles/most-events.tsxapps/start/src/components/profiles/popular-routes.tsxapps/start/src/components/profiles/profile-activity.tsxapps/start/src/components/profiles/profile-avatar.tsxapps/start/src/components/profiles/profile-metrics.tsxapps/start/src/components/profiles/profile-platforms.tsxapps/start/src/components/profiles/profile-properties.tsxapps/start/src/components/profiles/profile-source.tsxapps/start/src/components/profiles/table/columns.tsxapps/start/src/components/profiles/table/index.tsxapps/start/src/components/ui/data-table/data-table-hooks.tsxapps/start/src/components/ui/key-value-grid.tsxapps/start/src/routes/_app.$organizationId.$projectId.groups.tsxapps/start/src/routes/_app.$organizationId.$projectId.groups_.$groupId._tabs.index.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.profiles.$profileId._tabs.tsxapps/start/src/routes/_app.$organizationId.$projectId.profiles._tabs.anonymous.tsxapps/start/src/routes/_app.$organizationId.$projectId.profiles._tabs.identified.tsxapps/start/src/routes/_app.$organizationId.$projectId.profiles._tabs.power-users.tsxapps/start/src/utils/casing.tsapps/start/src/utils/date.tsapps/start/src/utils/source.tspackages/db/src/services/group.service.tspackages/db/src/services/profile.service.tspackages/trpc/src/routers/group.tspackages/trpc/src/routers/profile.tsscripts/seed-local.mjs
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
2 participants
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.
No description provided.