Feature/insights cards #263
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdds a full "insights" feature: frontend UI and route, TRPC endpoints, DB schema and migrations, a compute engine with multiple analysis modules, worker/cron jobs and queues, and supporting utilities for scoring, caching, and persistence. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Frontend UI
participant API as TRPC API
participant Worker as Cron/Worker
participant Engine as Insights Engine
participant CH as ClickHouse
participant Store as InsightStore (Postgres)
UI->>API: request insights (list / listAll)
API->>Store: query active insights (project)
Store->>Store: map/prepare payload
Store-->>API: insights JSON
API-->>UI: return insights
UI->>UI: render InsightCard carousel
Worker->>Worker: cron triggers insightsDaily
Worker->>Store: listProjectIdsForCadence('daily')
Store-->>Worker: project IDs
loop per project
Worker->>Worker: enqueue insightsProjectJob(projectId, date)
Worker->>Engine: runProject(projectId, cadence)
loop per module/window
Engine->>CH: fetch current & baseline aggregates
CH-->>Engine: aggregated rows
Engine->>Engine: computeMany -> RenderedCard & score
Engine->>Store: upsertInsight(card, metrics, decision)
Store->>Store: insert/update ProjectInsight, maybe insert InsightEvent
end
Engine->>Store: closeMissingActiveInsights(...)
Engine->>Store: applySuppression(...)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/db/code-migrations/8-order-keys.ts (2)
142-170: CRITICAL: Data loss risk when guard prevents migration.If the guard condition fails (created_at is missing or starts with '1970'), the data copy is skipped but table rename operations at lines 250-284 still execute unconditionally. This would:
- Create an empty
events_new_20251123table- Rename the populated
eventstable toevents_20251123(backup)- Rename the empty
events_new_20251123toevents(production)Result: Complete data loss — the production events table becomes empty while data sits in a backup table.
🔎 Apply this diff to skip the entire migration when guards fail:
+ let shouldMigrate = true; + const firstEventDateResponse = await chMigrationClient.query({ query: 'SELECT min(created_at) as created_at FROM events', format: 'JSONEachRow', }); const firstEventDateJson = await firstEventDateResponse.json<{ created_at: string; }>(); if ( firstEventDateJson[0]?.created_at && !firstEventDateJson[0]?.created_at.startsWith('1970') ) { const firstEventDate = new Date(firstEventDateJson[0]?.created_at); // Step 2: Copy data from old tables to new tables (partitioned by month for efficiency) // Set endDate to first of next month to ensure we capture all data in the current month const endDate = new Date(); endDate.setMonth(endDate.getMonth() + 1); endDate.setDate(1); sqls.push( ...moveDataBetweenTables({ from: 'events', to: 'events_new_20251123', batch: { startDate: firstEventDate, endDate: endDate, column: 'toDate(created_at)', interval: 'month', transform: (date: Date) => { const year = date.getFullYear(); const month = String(date.getMonth() + 1).padStart(2, '0'); return `${year}-${month}-01`; }, }, }), ); + } else { + console.warn('Skipping events migration: invalid or missing created_at'); + shouldMigrate = false; } const firstSessionDateResponse = await chMigrationClient.query({ query: 'SELECT min(created_at) as created_at FROM sessions', format: 'JSONEachRow', }); const firstSessionDateJson = await firstSessionDateResponse.json<{ created_at: string; }>(); if ( firstSessionDateJson[0]?.created_at && !firstSessionDateJson[0]?.created_at.startsWith('1970') ) { const firstSessionDate = new Date( firstSessionDateJson[0]?.created_at ?? '', ); // Set endDate to first of next month to ensure we capture all data in the current month const endDate = new Date(); endDate.setMonth(endDate.getMonth() + 1); endDate.setDate(1); sqls.push( ...moveDataBetweenTables({ from: 'sessions', to: 'sessions_new_20251123', columns: [ // ... columns ], batch: { startDate: firstSessionDate, endDate: endDate, column: 'toDate(created_at)', interval: 'month', transform: (date: Date) => { const year = date.getFullYear(); const month = String(date.getMonth() + 1).padStart(2, '0'); return `${year}-${month}-01`; }, }, }), ); + } else { + console.warn('Skipping sessions migration: invalid or missing created_at'); + shouldMigrate = false; } + + if (!shouldMigrate) { + console.error('Migration aborted due to invalid source data'); + return; + } sqls.push( ...renameTable({ from: 'events', to: 'events_20251123', isClustered }), ); // ... rest of rename operations
180-248: CRITICAL: Identical data loss risk for sessions table.This guard has the same fatal flaw as the events migration: if the condition fails, data copying is skipped but table rename operations (lines 250-284) proceed unconditionally, resulting in an empty production
sessionstable.Additionally, Line 185 uses nullish coalescing (
?? '') when constructing the Date, but this is inconsistent with the events migration (Line 146) which omits it. Since the guard already verifiedcreated_atexists, the fallback is unnecessary.Apply the same fix suggested in the previous comment to abort the entire migration when either guard fails.
sh/docker-build (1)
70-76: Logging messages don't reflect the actual tag for prerelease builds.Lines 71 and 75 always reference
$full_version, but for prerelease builds, the actual tag pushed is$full_version-$PRERELEASE. This causes the success/failure messages to display an incomplete tag name for prerelease builds.🔎 Apply this diff to fix the logging:
+ local tag_name="$full_version" + if [ -n "$PRERELEASE" ]; then + tag_name="$full_version-$PRERELEASE" + fi + if [ $? -ne 0 ]; then - echo "Failed to build $full_version" + echo "Failed to build $tag_name" exit 1 fi - echo "Successfully built and pushed multi-architecture image for $full_version" + echo "Successfully built and pushed multi-architecture image for $tag_name" }
🧹 Nitpick comments (30)
sh/docker-build (3)
41-41: Consider renamingfull_versionfor clarity.The variable
full_versioncontains the complete Docker image reference (registry/repository:tag), not just the version. A name likebase_tagorimage_tagwould be more accurate and improve maintainability.🔎 Suggested refactor:
- local full_version="$image_name:$VERSION" + local base_tag="$image_name:$VERSION"Then update all references to
full_versionthroughout the function to usebase_taginstead.
21-25: Consider showing the complete tag in the prerelease prompt.For prerelease builds, the prompt shows only
$VERSION, but the actual tag will be$VERSION-$PRERELEASE. Displaying the complete tag improves clarity and helps operators verify they're building the correct version.🔎 Suggested improvement:
if [ -z "$PRERELEASE" ]; then read -p "You're building WITHOUT any pre-release ($VERSION). Do you wish to continue (y/N)? " -n 1 -r else - read -p "You're building WITH pre-release ($VERSION). Do you wish to continue (y/N)? " -n 1 -r + read -p "You're building WITH pre-release ($VERSION-$PRERELEASE). Do you wish to continue (y/N)? " -n 1 -r fi
5-5: Add input validation for the PRERELEASE parameter.The PRERELEASE parameter is used directly in Docker tag construction without validation. Docker tags must contain only alphanumeric characters, dots, underscores, and hyphens, with a maximum of 128 characters. Invalid input will cause the docker buildx command to fail with a cryptic error.
🔎 Add validation after line 18:
exit 1 fi + +# Validate PRERELEASE format if provided +if [ -n "$PRERELEASE" ]; then + if [[ ! "$PRERELEASE" =~ ^[a-zA-Z0-9._-]+$ ]]; then + echo "Error: PRERELEASE tag '$PRERELEASE' contains invalid characters." + echo "Only alphanumeric characters, dots, underscores, and hyphens are allowed." + exit 1 + fi + if [ ${#PRERELEASE} -gt 128 ]; then + echo "Error: PRERELEASE tag is too long (max 128 characters)." + exit 1 + fi +fi # Add warning promptpackages/db/src/clickhouse/query-builder.ts (1)
37-43: Consider adding JSDoc to document Expression security implications.The
Expressionclass andclix.exp()function allow raw SQL to bypass escaping, which is powerful but risky. Consider adding JSDoc comments to warn developers:🔎 Suggested documentation additions:
+/** + * Represents a raw SQL expression that bypasses escaping. + * ⚠️ WARNING: Never pass user input to Expression. Only use with trusted SQL literals. + */ class Expression { constructor(private expression: string) {} toString() { return this.expression; } }+/** + * Creates a raw SQL expression that bypasses escaping. + * @param expr - A SQL expression string or Query object + * @returns An Expression object + * ⚠️ WARNING: Never pass user-supplied input to this function. + * Only use with trusted SQL literals to prevent SQL injection. + */ clix.exp = (expr: string | Query<any>) => new Expression(expr instanceof Query ? expr.toSQL() : expr);Also applies to: 663-664
packages/constants/index.ts (1)
501-503: Consider safer type handling for invalid country codes.The
getCountryfunction uses type assertion (as keyof typeof countries) which bypasses TypeScript's type checking. For invalid or undefined codes, this will returnundefinedat runtime, but the return type doesn't reflect this possibility.🔎 Apply this diff for safer type handling:
-export function getCountry(code?: string) { - return countries[code as keyof typeof countries]; +export function getCountry(code?: string): string | undefined { + if (!code) return undefined; + return countries[code as keyof typeof countries]; }This makes the return type explicit and adds a guard for undefined input, improving type safety and making the behavior clearer to consumers.
packages/db/prisma/schema.prisma (1)
507-541: Consider nullable displayName instead of empty string default.The schema is well-structured with appropriate indices and constraints. However,
displayNamehas@default("")which may create ambiguity between "not set" and "intentionally empty".🔎 Consider this alternative:
- displayName String @default("") + displayName String?This makes the "not set" state explicit and aligns with other optional string fields like
summaryanddirection. However, if your application logic specifically requires empty strings over null, the current approach is acceptable.packages/db/src/session-consistency.ts (1)
183-187: Consider the observability impact of removing this logging.The Prisma write operation logging has been commented out, which reduces visibility into database mutations. If this was removed to reduce log volume, consider:
- Using a higher log level (debug) instead of removing it entirely
- Adding an environment variable to conditionally enable detailed operation logging
- Documenting why this logging was removed
packages/db/src/prisma-client.ts (1)
45-49: Consider the observability impact of removing this logging.Similar to the change in
session-consistency.ts, this removes logging for Prisma operations (create, update, delete). This reduces visibility into database mutations across the application. If log volume is a concern, consider using conditional logging or higher log levels rather than complete removal.packages/db/src/services/insights/windows.ts (1)
44-58: Consider adding exhaustive type checking forWindowKind.The default block silently handles
rolling_30d, but if a newWindowKindis added to the type, this function will silently fall through without a compile-time error. Consider using an exhaustive check pattern.🔎 Suggested improvement
- // rolling_30d - { + if (kind === 'rolling_30d') { const end = yesterday0; const start = addDays(end, -29); const baselineEnd = addDays(start, -1); const baselineStart = addDays(baselineEnd, -29); return { kind, start, end, baselineStart, baselineEnd, label: 'Last 30 days', }; } + + // Exhaustive check - TypeScript will error if WindowKind is extended + const _exhaustive: never = kind; + throw new Error(`Unknown window kind: ${_exhaustive}`);apps/worker/src/jobs/insights.ts (1)
29-45: Unusedjobparameter ininsightsDailyJob.The
jobparameter is declared but never used. If you need access to job metadata for logging or other purposes, consider using it; otherwise, prefix with underscore to indicate intentional non-use.🔎 Suggested fix
-export async function insightsDailyJob(job: Job<CronQueuePayload>) { +export async function insightsDailyJob(_job: Job<CronQueuePayload>) {packages/db/src/services/insights/material.ts (2)
17-18: Avoidas anytype assertions.The
as anycasts bypass TypeScript's type checking. SincePersistedInsight.directionis typed asstring | null, consider narrowing with a type guard or updating the interface to use a union type.🔎 Suggested approach
- const prevDir = (prev.direction ?? 'flat') as any; - const nextDir = next.direction ?? 'flat'; + const prevDir = prev.direction ?? 'flat'; + const nextDir: string = next.direction ?? 'flat'; if (prevDir !== nextDir && (nextDir === 'up' || nextDir === 'down')) {Alternatively, consider updating
PersistedInsight.directionin types.ts to use'up' | 'down' | 'flat' | nullinstead ofstring | nullfor stricter typing.
27-35: Sameas anypattern forseverityBand.Apply the same type narrowing approach here for consistency.
🔎 Suggested fix
- const prevBand = (prev.severityBand ?? null) as any; + const prevBand = prev.severityBand ?? null;apps/start/src/components/overview/overview-insights.tsx (1)
20-26: Unusedfiltersvariable.The
filtersvalue fromuseEventQueryFilters()is destructured but never used. Consider removing it if not needed.🔎 Suggested fix
- const [filters, setFilter] = useEventQueryFilters(); + const [, setFilter] = useEventQueryFilters();packages/db/src/services/insights/modules/devices.module.ts (1)
260-266: Extract magic number as named constant.The threshold
0.0005for share direction detection should be a named constant for clarity and maintainability.🔎 Suggested fix
+const SHARE_DIRECTION_THRESHOLD = 0.0005; + // In render function: direction: - shareCurrent - shareCompare > 0.0005 + shareCurrent - shareCompare > SHARE_DIRECTION_THRESHOLD ? 'up' - : shareCurrent - shareCompare < -0.0005 + : shareCurrent - shareCompare < -SHARE_DIRECTION_THRESHOLD ? 'down' : 'flat',packages/queue/src/queues.ts (1)
248-256: Consider addingremoveOnFailfor consistency withimportQueue.The
insightsQueueis well-structured withremoveOnComplete: 100for extended job retention. However,importQueueincludesremoveOnFail: 50to preserve failed jobs for debugging. Adding a similarremoveOnFailoption would help with troubleshooting failed insight jobs and align with the pattern used inimportQueue.packages/trpc/src/routers/insight.ts (1)
30-32: Hardcoded module exclusion may need documentation.The
listprocedure excludespage-trendsandentry-pagesmodules. Consider adding a comment explaining why these modules are excluded from the main list, or extracting this to a constant for easier maintenance.🔎 Suggested improvement:
+// These modules are excluded from the main insights list +// because they may generate too many similar cards +const EXCLUDED_MODULES = ['page-trends', 'entry-pages']; + const allInsights = await db.projectInsight.findMany({ where: { projectId, state: 'active', moduleKey: { - notIn: ['page-trends', 'entry-pages'], + notIn: EXCLUDED_MODULES, }, },packages/db/src/services/insights/modules/referrers.module.ts (1)
259-264: Hardcoded share direction threshold differs from pattern in other metrics.The share direction calculation uses a hardcoded threshold of
0.0005(0.05%), while other modules usecomputeDirectionwith a default threshold. Consider extracting this to a constant or using a consistent approach.🔎 Suggested improvement:
+const SHARE_DIRECTION_THRESHOLD = 0.0005; // 0.05% + direction: - shareCurrent - shareCompare > 0.0005 + shareCurrent - shareCompare > SHARE_DIRECTION_THRESHOLD ? 'up' - : shareCurrent - shareCompare < -0.0005 + : shareCurrent - shareCompare < -SHARE_DIRECTION_THRESHOLD ? 'down' : 'flat',packages/db/src/services/insights/modules/entry-pages.module.ts (1)
20-172: Significant code duplication withpage-trends.module.ts.The
fetchEntryPageAggregatesfunction is structurally almost identical tofetchPageTrendAggregatesinpage-trends.module.ts, differing only in:
- Table source (
sessionsvsevents)- Column names (
entry_origin/entry_pathvsorigin/path)- Event filter (
sign=1vsname='screen_view')Consider extracting a generic aggregation helper to reduce duplication. This is a recommended refactor that could be deferred.
#!/bin/bash # Show the similarity between the two fetch functions echo "=== page-trends fetchPageTrendAggregates ===" rg -A 30 "async function fetchPageTrendAggregates" --type ts echo "" echo "=== entry-pages fetchEntryPageAggregates ===" rg -A 30 "async function fetchEntryPageAggregates" --type tspackages/db/src/services/insights/engine.ts (3)
59-68: Consider stronger typing fordbparameter.The
dbparameter is typed asany, which loses type safety. Consider using the actual database client type from your Prisma setup.🔎 Suggested improvement
+import type { PrismaClient } from '../../prisma-client'; + export function createEngine(args: { store: InsightStore; modules: InsightModule[]; - db: any; + db: PrismaClient; logger?: Pick<Console, 'info' | 'warn' | 'error'>; config: EngineConfig; }) {
193-198: Potential N+1 query pattern for previous insight lookups.Each result triggers a separate
getActiveInsightByIdentitycall. For high-volume scenarios, consider batch-fetching previous insights for all dimensionKeys in the batch before the loop.The current approach is acceptable given the
maxDimscap (default 25), but batching could reduce database round-trips if performance becomes a concern.
246-254: Consider extracting event kind determination to a helper function.The nested ternary for computing
eventKindis functional but hard to read. A small helper would improve clarity.🔎 Suggested refactor
function determineEventKind( decision: { reason: string }, sev: string | null, prevSeverity: string | null ): string { if (decision.reason === 'direction_flip') return 'direction_flip'; if (decision.reason === 'severity_change') { return sev && prevSeverity && sev > prevSeverity ? 'severity_up' : 'severity_down'; } return 'updated'; }packages/db/src/services/insights/modules/geo.module.ts (1)
256-261: Consider extracting the share direction threshold as a named constant.The magic number
0.0005for share direction determination is used inline. A named constant would improve readability and make it easier to tune.🔎 Suggested improvement
+const SHARE_DIRECTION_THRESHOLD = 0.0005; + // In render function: direction: - shareCurrent - shareCompare > 0.0005 + shareCurrent - shareCompare > SHARE_DIRECTION_THRESHOLD ? 'up' - : shareCurrent - shareCompare < -0.0005 + : shareCurrent - shareCompare < -SHARE_DIRECTION_THRESHOLD ? 'down' : 'flat',packages/db/src/services/insights/utils.ts (1)
40-48: Type safety could be improved with generics constraint.The
(row as any).dateand(row as any).cntcasts bypass type checking. Consider adding a constraint to the generic typeTto require these properties.🔎 Suggested improvement
-export function computeWeekdayMedians<T>( +interface DailyRow { + date: string; + cnt: number; +} + +export function computeWeekdayMedians<T extends DailyRow>( data: T[], targetWeekday: number, getDimension: (row: T) => string, ): Map<string, number> { // ... for (const row of data) { - const rowWeekday = getWeekday(new Date((row as any).date)); + const rowWeekday = getWeekday(new Date(row.date)); // ... - values.push(Number((row as any).cnt ?? 0)); + values.push(Number(row.cnt ?? 0));apps/start/src/routes/_app.$organizationId.$projectId.insights.tsx (2)
171-189: DuplicateseverityOrdermap definition.The
severityOrderrecord is defined identically in bothseverity-descandseverity-asccases. Extract it outside the switch for DRY.🔎 Suggested improvement
+const SEVERITY_ORDER: Record<string, number> = { + severe: 3, + moderate: 2, + low: 1, +}; + // Inside the sort function: case 'severity-desc': { - const severityOrder: Record<string, number> = { - severe: 3, - moderate: 2, - low: 1, - }; - const aSev = severityOrder[a.severityBand ?? ''] ?? 0; - const bSev = severityOrder[b.severityBand ?? ''] ?? 0; + const aSev = SEVERITY_ORDER[a.severityBand ?? ''] ?? 0; + const bSev = SEVERITY_ORDER[b.severityBand ?? ''] ?? 0; return bSev - aSev; } case 'severity-asc': { - const severityOrder: Record<string, number> = { ... }; - const aSev = severityOrder[a.severityBand ?? ''] ?? 0; - const bSev = severityOrder[b.severityBand ?? ''] ?? 0; + const aSev = SEVERITY_ORDER[a.severityBand ?? ''] ?? 0; + const bSev = SEVERITY_ORDER[b.severityBand ?? ''] ?? 0; return aSev - bSev; }
391-410: Consider simplifying the onFilter callback creation.The IIFE pattern for creating
onFilterworks but is somewhat unusual. A simpler approach would be a helper function or inline conditional.🔎 Alternative approach
const getFilterHandler = (insight: typeof moduleInsights[0]) => { const filterString = insight.payload?.dimensions .map((dim) => `${dim.key},is,${encodeURIComponent(dim.value)}`) .join(';'); if (!filterString) return undefined; return () => { navigate({ to: '/$organizationId/$projectId', from: Route.fullPath, search: { f: filterString }, }); }; }; // Usage: <InsightCard insight={insight} onFilter={getFilterHandler(insight)} />packages/db/src/services/insights/store.ts (2)
269-275: Individual updates in loop could be batched.The suppression logic performs individual
db.projectInsight.updatecalls in loops. For better performance at scale, consider batching these updates.🔎 Suggested approach for stale insights
// Instead of looping with individual updates: const staleIds = insights .filter(i => isStale(i) && i.state === 'active') .map(i => i.id); if (staleIds.length > 0) { const result = await db.projectInsight.updateMany({ where: { id: { in: staleIds } }, data: { state: 'suppressed', lastUpdatedAt: now }, }); suppressed += result.count; }Apply similar batching for
belowNandtopNupdates.
113-155: Type safety lost withanydeclaration.The
insightvariable is typed asany, losing Prisma's type inference from the create/update operations. Consider using the actual Prisma type.🔎 Suggested fix
-let insight: any; +let insight: Awaited<ReturnType<typeof db.projectInsight.create>>;Or import the generated Prisma type:
import type { ProjectInsight } from '@prisma/client'; // ... let insight: ProjectInsight;packages/db/prisma/migrations/20251212192459_insights/migration.sql (2)
50-51: Note: Unique constraint includesstate— allows multiple rows per logical insight.This design enables keeping historical closed/suppressed rows while creating new active ones. Consider adding a periodic cleanup job to archive or delete old closed insights to prevent table bloat over time.
5-30: Add foreign key constraint to maintain consistency with other tables.The
project_insightstable lacks a foreign key constraint onprojectId, while similar tables (imports,chats) defineFOREIGN KEY (projectId) REFERENCES projects(id) ON DELETE CASCADE. To maintain referential integrity and codebase consistency, add:ALTER TABLE "public"."project_insights" ADD CONSTRAINT "project_insights_projectId_fkey" FOREIGN KEY ("projectId") REFERENCES "public"."projects"("id") ON DELETE CASCADE ON UPDATE CASCADE;packages/db/src/services/insights/types.ts (1)
21-33: Consider adding a more specific type fordbinstead ofany.Using
anyfor the database client loses type safety. If this is a Prisma client, consider:export interface ComputeContext { projectId: string; window: WindowRange; - db: any; // your DB client + db: import('@prisma/client').PrismaClient; // or a more specific subset interface now: Date; logger: Pick<Console, 'info' | 'warn' | 'error'>;Alternatively, if the engine needs to remain DB-agnostic, define a minimal interface with just the methods used.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (48)
apps/start/src/components/insights/insight-card.tsx(1 hunks)apps/start/src/components/overview/overview-insights.tsx(1 hunks)apps/start/src/components/sidebar-project-menu.tsx(2 hunks)apps/start/src/components/sidebar.tsx(1 hunks)apps/start/src/components/ui/carousel.tsx(1 hunks)apps/start/src/routeTree.gen.ts(11 hunks)apps/start/src/routes/_app.$organizationId.$projectId.index.tsx(2 hunks)apps/start/src/routes/_app.$organizationId.$projectId.insights.tsx(1 hunks)apps/start/src/utils/title.ts(1 hunks)apps/worker/src/boot-cron.ts(1 hunks)apps/worker/src/boot-workers.ts(4 hunks)apps/worker/src/index.ts(2 hunks)apps/worker/src/jobs/cron.ts(2 hunks)apps/worker/src/jobs/insights.ts(1 hunks)packages/constants/index.ts(1 hunks)packages/db/code-migrations/4-add-sessions.ts(1 hunks)packages/db/code-migrations/8-order-keys.ts(2 hunks)packages/db/index.ts(1 hunks)packages/db/prisma/migrations/20251212192459_insights/migration.sql(1 hunks)packages/db/prisma/migrations/20251217204808_insight_payload_default/migration.sql(1 hunks)packages/db/prisma/migrations/20251217210920_insights/migration.sql(1 hunks)packages/db/prisma/schema.prisma(1 hunks)packages/db/src/clickhouse/query-builder.ts(3 hunks)packages/db/src/prisma-client.ts(1 hunks)packages/db/src/services/insights/cached-clix.ts(1 hunks)packages/db/src/services/insights/engine.ts(1 hunks)packages/db/src/services/insights/index.ts(1 hunks)packages/db/src/services/insights/material.ts(1 hunks)packages/db/src/services/insights/modules/devices.module.ts(1 hunks)packages/db/src/services/insights/modules/entry-pages.module.ts(1 hunks)packages/db/src/services/insights/modules/geo.module.ts(1 hunks)packages/db/src/services/insights/modules/index.ts(1 hunks)packages/db/src/services/insights/modules/page-trends.module.ts(1 hunks)packages/db/src/services/insights/modules/referrers.module.ts(1 hunks)packages/db/src/services/insights/scoring.ts(1 hunks)packages/db/src/services/insights/store.ts(1 hunks)packages/db/src/services/insights/types.ts(1 hunks)packages/db/src/services/insights/utils.ts(1 hunks)packages/db/src/services/insights/windows.ts(1 hunks)packages/db/src/session-consistency.ts(1 hunks)packages/db/src/types.ts(2 hunks)packages/queue/src/queues.ts(2 hunks)packages/trpc/src/root.ts(2 hunks)packages/trpc/src/routers/insight.ts(1 hunks)packages/validation/index.ts(1 hunks)packages/validation/src/index.ts(1 hunks)packages/validation/src/types.insights.ts(1 hunks)sh/docker-build(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursorrules)
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
Files:
packages/db/src/services/insights/modules/index.tspackages/validation/src/index.tspackages/db/src/prisma-client.tspackages/db/index.tsapps/start/src/routes/_app.$organizationId.$projectId.index.tsxapps/start/src/components/insights/insight-card.tsxpackages/trpc/src/root.tspackages/db/src/services/insights/index.tspackages/db/src/services/insights/windows.tspackages/validation/index.tsapps/start/src/components/overview/overview-insights.tsxapps/worker/src/boot-cron.tspackages/db/src/session-consistency.tspackages/db/src/services/insights/material.tsapps/worker/src/jobs/cron.tsapps/start/src/components/sidebar-project-menu.tsxpackages/db/src/services/insights/modules/geo.module.tsapps/start/src/components/sidebar.tsxapps/worker/src/jobs/insights.tspackages/db/src/services/insights/modules/entry-pages.module.tspackages/db/src/services/insights/cached-clix.tspackages/trpc/src/routers/insight.tspackages/db/src/services/insights/modules/referrers.module.tspackages/constants/index.tsapps/start/src/components/ui/carousel.tsxpackages/db/src/services/insights/scoring.tspackages/db/code-migrations/8-order-keys.tspackages/db/src/services/insights/utils.tspackages/db/src/services/insights/modules/page-trends.module.tspackages/db/code-migrations/4-add-sessions.tspackages/db/src/clickhouse/query-builder.tsapps/start/src/routes/_app.$organizationId.$projectId.insights.tsxapps/start/src/utils/title.tspackages/db/src/services/insights/store.tsapps/worker/src/index.tspackages/queue/src/queues.tspackages/validation/src/types.insights.tspackages/db/src/services/insights/engine.tspackages/db/src/services/insights/modules/devices.module.tsapps/start/src/routeTree.gen.tspackages/db/src/services/insights/types.tsapps/worker/src/boot-workers.tspackages/db/src/types.ts
apps/start/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/start/.cursorrules)
Instrument server functions with Sentry using
Sentry.startSpan()to wrap the implementation. Import Sentry with:import * as Sentry from '@sentry/tanstackstart-react'
Files:
apps/start/src/routes/_app.$organizationId.$projectId.index.tsxapps/start/src/components/insights/insight-card.tsxapps/start/src/components/overview/overview-insights.tsxapps/start/src/components/sidebar-project-menu.tsxapps/start/src/components/sidebar.tsxapps/start/src/components/ui/carousel.tsxapps/start/src/routes/_app.$organizationId.$projectId.insights.tsxapps/start/src/utils/title.tsapps/start/src/routeTree.gen.ts
🧠 Learnings (12)
📚 Learning: 2025-12-15T09:19:40.525Z
Learnt from: CR
Repo: Openpanel-dev/openpanel PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-15T09:19:40.525Z
Learning: Applies to **/*.{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`
Applied to files:
packages/db/index.tspackages/db/src/services/insights/cached-clix.tspackages/db/src/clickhouse/query-builder.ts
📚 Learning: 2025-12-15T09:20:11.883Z
Learnt from: CR
Repo: Openpanel-dev/openpanel PR: 0
File: apps/start/.cursorrules:0-0
Timestamp: 2025-12-15T09:20:11.883Z
Learning: Applies to apps/start/routes/__root.tsx : Use `createRootRouteWithContext<T>()` to provide typed context to the router that can be accessed in loaders and other route functions
Applied to files:
packages/trpc/src/root.ts
📚 Learning: 2025-12-15T09:20:11.883Z
Learnt from: CR
Repo: Openpanel-dev/openpanel PR: 0
File: apps/start/.cursorrules:0-0
Timestamp: 2025-12-15T09:20:11.883Z
Learning: Applies to apps/start/src/router.tsx : Error collection is automatically configured in `src/router.tsx` - do not duplicate Sentry error collection configuration elsewhere
Applied to files:
packages/trpc/src/root.ts
📚 Learning: 2025-12-15T09:20:11.883Z
Learnt from: CR
Repo: Openpanel-dev/openpanel PR: 0
File: apps/start/.cursorrules:0-0
Timestamp: 2025-12-15T09:20:11.883Z
Learning: Applies to apps/start/routes/**/*.{ts,tsx} : Define routes using `createFileRoute(path)` with the path auto-managed by the bundler/CLI
Applied to files:
apps/start/src/routes/_app.$organizationId.$projectId.insights.tsxapps/start/src/routeTree.gen.ts
📚 Learning: 2025-12-15T09:20:11.883Z
Learnt from: CR
Repo: Openpanel-dev/openpanel PR: 0
File: apps/start/.cursorrules:0-0
Timestamp: 2025-12-15T09:20:11.883Z
Learning: Applies to apps/start/routes/**/*.{ts,tsx} : Un-nest routes by suffixing a segment with `_` (e.g., `posts_.$postId.edit.tsx`) to render independently from parent routes
Applied to files:
apps/start/src/routes/_app.$organizationId.$projectId.insights.tsxapps/start/src/routeTree.gen.ts
📚 Learning: 2025-12-15T09:20:11.883Z
Learnt from: CR
Repo: Openpanel-dev/openpanel PR: 0
File: apps/start/.cursorrules:0-0
Timestamp: 2025-12-15T09:20:11.883Z
Learning: Applies to apps/start/routes/**/*.{ts,tsx} : Use a trailing slash in `createFileRoute()` for index routes that target the exact parent path (e.g., `/posts/`)
Applied to files:
apps/start/src/routes/_app.$organizationId.$projectId.insights.tsxapps/start/src/routeTree.gen.ts
📚 Learning: 2025-12-15T09:20:11.883Z
Learnt from: CR
Repo: Openpanel-dev/openpanel PR: 0
File: apps/start/.cursorrules:0-0
Timestamp: 2025-12-15T09:20:11.883Z
Learning: Applies to apps/start/routes/**/*.{ts,tsx} : Use `<Outlet/>` component in layout routes to render child route components
Applied to files:
apps/start/src/routes/_app.$organizationId.$projectId.insights.tsxapps/start/src/routeTree.gen.ts
📚 Learning: 2025-12-15T09:20:11.883Z
Learnt from: CR
Repo: Openpanel-dev/openpanel PR: 0
File: apps/start/.cursorrules:0-0
Timestamp: 2025-12-15T09:20:11.883Z
Learning: Applies to apps/start/routes/**/*.{ts,tsx} : Use `(group)` folder names to organize routes for file structure only, without affecting URL paths or nesting
Applied to files:
apps/start/src/routeTree.gen.ts
📚 Learning: 2025-12-15T09:20:11.883Z
Learnt from: CR
Repo: Openpanel-dev/openpanel PR: 0
File: apps/start/.cursorrules:0-0
Timestamp: 2025-12-15T09:20:11.883Z
Learning: Applies to apps/start/routes/**/*.{ts,tsx} : Prefix files and folders with `-` to exclude them from route generation and enable safe file colocation (e.g., `-posts-table.tsx`, `-components/`)
Applied to files:
apps/start/src/routeTree.gen.ts
📚 Learning: 2025-12-15T09:20:11.883Z
Learnt from: CR
Repo: Openpanel-dev/openpanel PR: 0
File: apps/start/.cursorrules:0-0
Timestamp: 2025-12-15T09:20:11.883Z
Learning: Applies to apps/start/routes/**/*.{ts,tsx} : Use pathless layout routes by prefixing filenames with `_` (e.g., `_settingsShell`) to wrap children without matching URL segments
Applied to files:
apps/start/src/routeTree.gen.ts
📚 Learning: 2025-12-15T09:20:11.883Z
Learnt from: CR
Repo: Openpanel-dev/openpanel PR: 0
File: apps/start/.cursorrules:0-0
Timestamp: 2025-12-15T09:20:11.883Z
Learning: Applies to apps/start/routes/**/*.{ts,tsx} : Use a lone `$` segment for splat (catch-all) routes to capture the rest of the path into `params._splat`
Applied to files:
apps/start/src/routeTree.gen.ts
📚 Learning: 2025-12-15T09:20:11.883Z
Learnt from: CR
Repo: Openpanel-dev/openpanel PR: 0
File: apps/start/.cursorrules:0-0
Timestamp: 2025-12-15T09:20:11.883Z
Learning: Applies to apps/start/routes/**/*.{ts,tsx} : Use dynamic segments with `$param` syntax (e.g., `/posts/$postId`) for parameterized routes
Applied to files:
apps/start/src/routeTree.gen.ts
🧬 Code graph analysis (18)
apps/start/src/routes/_app.$organizationId.$projectId.index.tsx (1)
apps/start/src/components/overview/overview-insights.tsx (1)
OverviewInsights(18-75)
packages/trpc/src/root.ts (1)
packages/trpc/src/routers/insight.ts (1)
insightRouter(7-102)
packages/db/src/services/insights/windows.ts (1)
packages/db/src/services/insights/types.ts (2)
WindowKind(10-10)WindowRange(12-19)
packages/db/src/services/insights/material.ts (1)
packages/db/src/services/insights/types.ts (2)
PersistedInsight(95-108)MaterialDecision(119-123)
apps/worker/src/jobs/cron.ts (1)
apps/worker/src/jobs/insights.ts (1)
insightsDailyJob(29-45)
packages/db/src/services/insights/modules/entry-pages.module.ts (3)
packages/db/src/services/insights/types.ts (4)
ComputeContext(21-33)InsightModule(71-92)ComputeResult(35-43)RenderedCard(56-61)packages/db/src/clickhouse/client.ts (2)
TABLE_NAMES(50-62)formatClickhouseDate(281-292)packages/db/src/services/insights/utils.ts (7)
getEndOfDay(92-96)buildLookupMap(107-119)getWeekday(8-10)computeWeekdayMedians(32-58)selectTopDimensions(130-151)computeChangePct(63-72)computeDirection(77-86)
packages/db/src/services/insights/cached-clix.ts (1)
packages/db/src/clickhouse/query-builder.ts (1)
Query(45-609)
packages/trpc/src/routers/insight.ts (3)
packages/trpc/src/trpc.ts (2)
createTRPCRouter(134-134)protectedProcedure(166-170)packages/trpc/src/errors.ts (1)
TRPCAccessError(3-7)packages/db/src/prisma-client.ts (1)
db(292-292)
packages/db/src/services/insights/modules/referrers.module.ts (3)
packages/db/src/services/insights/types.ts (4)
ComputeContext(21-33)InsightModule(71-92)ComputeResult(35-43)RenderedCard(56-61)packages/db/src/clickhouse/client.ts (2)
TABLE_NAMES(50-62)formatClickhouseDate(281-292)packages/db/src/services/insights/utils.ts (7)
getEndOfDay(92-96)buildLookupMap(107-119)getWeekday(8-10)computeWeekdayMedians(32-58)selectTopDimensions(130-151)computeChangePct(63-72)computeDirection(77-86)
packages/db/src/services/insights/scoring.ts (1)
packages/db/src/services/insights/types.ts (1)
ComputeResult(35-43)
packages/db/src/services/insights/utils.ts (1)
packages/db/src/clickhouse/query-builder.ts (1)
end(643-655)
apps/start/src/routes/_app.$organizationId.$projectId.insights.tsx (6)
packages/common/src/math.ts (1)
sum(20-21)apps/start/src/components/ui/carousel.tsx (5)
Carousel(259-259)CarouselContent(260-260)CarouselItem(261-261)CarouselPrevious(262-262)CarouselNext(263-263)apps/start/src/components/ui/table.tsx (1)
TableButtons(5-17)apps/public/src/components/ui/input.tsx (1)
Input(39-39)apps/start/src/components/ui/select.tsx (5)
Select(173-173)SelectTrigger(181-181)SelectValue(182-182)SelectContent(174-174)SelectItem(176-176)apps/start/src/components/insights/insight-card.tsx (1)
InsightCard(29-199)
apps/worker/src/index.ts (1)
packages/queue/src/queues.ts (1)
insightsQueue(248-256)
packages/queue/src/queues.ts (1)
packages/redis/redis.ts (1)
getRedisQueue(93-105)
packages/db/src/services/insights/modules/devices.module.ts (2)
packages/db/src/clickhouse/client.ts (2)
TABLE_NAMES(50-62)formatClickhouseDate(281-292)packages/db/src/services/insights/utils.ts (7)
getEndOfDay(92-96)buildLookupMap(107-119)getWeekday(8-10)computeMedian(15-21)selectTopDimensions(130-151)computeChangePct(63-72)computeDirection(77-86)
packages/db/src/services/insights/types.ts (1)
packages/validation/src/types.insights.ts (1)
InsightPayload(33-43)
apps/worker/src/boot-workers.ts (2)
packages/queue/src/queues.ts (1)
insightsQueue(248-256)apps/worker/src/jobs/insights.ts (1)
insightsProjectJob(47-72)
packages/db/src/types.ts (1)
packages/validation/src/types.insights.ts (1)
InsightPayload(33-43)
🔇 Additional comments (44)
apps/start/src/components/sidebar.tsx (1)
126-126: LGTM! Clean UI improvement.The
hide-scrollbarclass addition creates a cleaner visual appearance while maintaining scrolling functionality viaoverflow-auto. Keyboard navigation and mouse/touch scrolling remain fully functional.sh/docker-build (1)
49-57: LGTM! Prerelease tagging logic is correct.The prerelease tag format
$full_version-$PRERELEASEcorrectly produces tags like "lindesvard/openpanel-app:1.0.0-beta". The implementation properly appends the prerelease identifier to the version tag.packages/db/code-migrations/4-add-sessions.ts (1)
103-105: Critical: Use the custom ClickHouse query builder instead of raw SQL strings.The coding guidelines require all ClickHouse queries to use the custom query builder from
./packages/db/src/clickhouse/query-builder.tsand./packages/db/src/clickhouse/query-functions.ts. This code bypasses that requirement by constructing a raw SQL string and callingchMigrationClient.query()directly.Refactor the query construction to use the custom query builder to ensure consistency and maintainability across the codebase.
While the epoch guard logic itself is sound (the
startsWith('1970')check is valid since ClickHouse returns DateTime values in YYYY-MM-DD hh:mm:ss format by default), addressing the query builder requirement is mandatory.⛔ Skipped due to learnings
Learnt from: CR Repo: Openpanel-dev/openpanel PR: 0 File: .cursorrules:0-0 Timestamp: 2025-12-15T09:19:40.525Z Learning: Applies to **/*.{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`packages/db/src/clickhouse/query-builder.ts (1)
83-98: LGTM!The method signature and implementation correctly handle both
stringandExpressiontypes. The type guards properly filter out falsy values while preserving both strings and Expression objects.apps/start/src/components/sidebar-project-menu.tsx (1)
20-20: LGTM! Clean navigation integration.The Insights link is properly integrated into the sidebar with appropriate icon and routing. The section header change from "Insights" to "Analytics" provides clearer distinction between the analytics section and the new Insights feature.
Also applies to: 43-43, 50-54
apps/worker/src/jobs/cron.ts (1)
9-9: LGTM! Consistent cron job integration.The new
insightsDailycase handler follows the established pattern and correctly delegates to theinsightsDailyJobfunction.Also applies to: 31-33
packages/constants/index.ts (1)
249-499: LGTM! Comprehensive country mapping.The countries object provides a thorough mapping of ISO country codes to full names, supporting internationalization needs for the geo insights module.
packages/db/prisma/schema.prisma (2)
501-505: LGTM! Well-defined insight state enum.The
InsightStateenum provides clear states for insight lifecycle management (active, suppressed, closed).
543-554: LGTM! Proper audit trail for insights.The
InsightEventmodel provides good event tracking with appropriate cascade behavior and indexing for chronological queries.packages/validation/src/types.insights.ts (1)
1-43: LGTM! Well-structured insight types with good compatibility design.The type definitions are comprehensive and well-thought-out:
- Clear metric types and units
- Flexible
InsightExtrawith index signature for module-specific datakindfield inInsightPayloadenables versioning for backwards compatibility- Appropriate use of
Partial<Record<...>>for optional metricsThe comment noting that this payload is "embedded in DB rows and shipped to the frontend, so it must remain backwards compatible" shows good architectural awareness.
apps/start/src/components/ui/carousel.tsx (1)
211-211: LGTM! Minor whitespace cleanup.Removed unnecessary space in className string. No functional impact.
apps/worker/src/index.ts (1)
9-9: LGTM! Proper queue monitoring integration.The
insightsQueueis correctly wired into the Bull Board UI for monitoring, following the established pattern for other queues.Also applies to: 46-46
packages/validation/index.ts (1)
3-3: LGTM!The export follows the existing pattern and properly exposes the insights types from the validation package.
packages/db/index.ts (1)
31-31: LGTM!The export follows the established pattern for service exports and properly exposes the insights service from the db package.
packages/db/prisma/migrations/20251217210920_insights/migration.sql (1)
1-13: Verify that the column removals are safe for production.This migration drops three columns (
changePct,compareValue,currentValue) from theproject_insightstable, which will result in data loss if these columns contain data. Ensure that:
- These columns are no longer needed in the new insights implementation
- Any existing data has been migrated or is no longer required
- This change is coordinated with the application deployment to avoid runtime errors
packages/db/src/types.ts (2)
6-6: LGTM!The import properly brings in the InsightPayload type from the validation package.
22-22: LGTM!The type alias correctly maps the InsightPayload to the Prisma JSON type namespace, following the established pattern for other JSON types in this file.
apps/start/src/utils/title.ts (1)
93-93: LGTM!The INSIGHTS page title constant is added appropriately and follows the existing pattern.
apps/worker/src/boot-cron.ts (1)
37-41: LGTM! Consider documenting the timezone.The insights daily cron job is properly configured following the existing pattern. The schedule
'0 2 * * *'will run at 2 AM daily, which is appropriate for background processing during low-traffic hours.Note that cron patterns use the server's timezone. If timezone-specific scheduling is important, consider documenting the expected timezone or using a library that supports explicit timezone configuration.
packages/trpc/src/root.ts (1)
8-8: LGTM! Clean router integration.The insight router is properly imported and registered following the established pattern used by all other routers in the application.
Also applies to: 51-51
packages/validation/src/index.ts (1)
557-557: LGTM! Consistent barrel export.The re-export follows the established pattern in this file and properly exposes insights validation types through the package's public API.
packages/db/src/services/insights/index.ts (1)
1-8: LGTM! Clean barrel export pattern.This index file properly consolidates all insights service exports into a single entry point, following best practices for module organization.
apps/start/src/routes/_app.$organizationId.$projectId.index.tsx (1)
6-6: LGTM! Proper component integration.The OverviewInsights component is cleanly integrated into the dashboard grid, with the projectId correctly passed from route parameters.
Also applies to: 54-54
packages/db/src/services/insights/modules/index.ts (1)
1-5: LGTM! Clean module exports.The barrel export properly consolidates all insight module exports, providing a clean import surface for consumers.
apps/start/src/components/insights/insight-card.tsx (1)
11-21: LGTM! Safe null handling and rendering logic.The helper functions (
formatWindowKind,formatValue,renderTitle) andDeltaChipcomponent use proper null handling with optional chaining and fallback values. The conditional rendering logic is well-structured.Also applies to: 59-96, 201-229
packages/db/src/services/insights/scoring.ts (1)
3-18: LGTM! Robust scoring implementation.The scoring functions use safe mathematical operations with proper null handling. The
log1ptransformation and severity thresholds provide a reasonable heuristic for prioritizing insights by impact.apps/worker/src/boot-workers.ts (1)
10-10: LGTM! Consistent worker integration.The insights worker is properly integrated following the established pattern used by all other workers. The default concurrency of 5 is reasonable for insights processing, and the worker includes proper lifecycle event handling.
Also applies to: 25-25, 54-62, 200-209
apps/worker/src/jobs/insights.ts (1)
47-72: LGTM!The
insightsProjectJobimplementation correctly extracts payload data, builds the engine with appropriate modules, and runs the analysis. The idempotent job IDs in the daily job ensure safe re-runs.apps/start/src/components/overview/overview-insights.tsx (2)
28-46: LGTM - loading state implementation.The skeleton carousel provides good UX during loading with appropriate responsive breakpoints matching the main carousel.
61-65: setFilter is synchronous, not asynchronous.The
setFilterfunction fromuseEventQueryFiltersis a synchronous state setter created withuseCallback. It does not return a Promise, so the concern about unhandled promises in theforEachloop is invalid. The current pattern is correct for iterating over dimensions and applying synchronous filter updates.Likely an incorrect or invalid review comment.
packages/db/src/services/insights/modules/devices.module.ts (1)
225-274: LGTM - render implementation.The render function correctly constructs the insight card with appropriate metrics, direction indicators, and payload structure for the insight_v1 format.
packages/db/src/services/insights/cached-clix.ts (1)
1-68: LGTM! Clean caching wrapper implementation.The caching mechanism correctly:
- Uses SHA-256 hash of SQL + timezone as cache key to ensure query uniqueness
- Preserves the original clix API by copying static methods
- Handles optional cache gracefully
- Uses the custom query builder as per coding guidelines
packages/db/src/services/insights/modules/page-trends.module.ts (2)
233-297: LGTM!The render function correctly:
- Extracts absolute pageviews from
extrasincecurrentValue/compareValueare now share-based- Handles the
isNewcase with appropriate title formatting- Constructs a well-structured payload with both pageviews and share metrics
173-185: No actionable issues found. The code correctly uses a shared cache context viacreateCachedClix, which ensures identical queries withinenumerateDimensionsandcomputeManyare deduplicated. Both methods receive the samectxobject with the same cached clix instance, guaranteeing cache hits for duplicate queries.packages/trpc/src/routers/insight.ts (1)
72-101: LGTM!The
listAllprocedure provides a straightforward way to fetch all active insights without the deduplication logic, which is useful for debugging or administrative views.packages/db/src/services/insights/modules/referrers.module.ts (1)
159-211: LGTM!The referrers module correctly:
- Implements the InsightModule interface with appropriate thresholds
- Tracks both
isNewandisGonereferrers in the extra field- Uses the cached clix query builder per coding guidelines
- Follows the established pattern from other modules
packages/db/src/services/insights/modules/entry-pages.module.ts (1)
174-287: LGTM!The entry pages module correctly implements the InsightModule interface with:
- Appropriate thresholds for entry page analysis
- Proper handling of origin/path dimensions
- Well-structured render output with sessions and share metrics
Note: Unlike the referrers module, this doesn't track
isGoneentries. This may be intentional since entry pages appearing/disappearing might be less actionable than referrer changes.packages/db/src/services/insights/utils.ts (2)
15-21: LGTM - computeMedian handles edge cases correctly.The function properly handles empty arrays and correctly computes median for both odd and even-length arrays. The null-coalescing fallbacks provide safe defaults.
130-151: LGTM - selectTopDimensions correctly implements union ranking.The function properly merges dimensions from both maps and ranks by the maximum value across current/baseline periods, ensuring dimensions significant in either period are included.
packages/db/src/services/insights/store.ts (1)
100-111: LGTM - Insight lookup and reopen logic is well-structured.The upsert logic correctly handles three cases: updating existing active insights, reopening closed/suppressed insights, and creating new ones. ThreadId preservation ensures continuity.
packages/db/src/services/insights/modules/geo.module.ts (1)
165-177: No action needed—duplicate queries are already handled by the caching mechanism.Both
enumerateDimensionsandcomputeManyreceive the sameComputeContextwith a shared cache Map. WhenfetchGeoAggregatesexecutes its queries viactx.clix(), the first call caches results by query hash, and the second call reuses them. No duplicate queries reach ClickHouse within a module+window execution.apps/start/src/routeTree.gen.ts (1)
1-10: Auto-generated file — no manual review required.This file is auto-generated by TanStack Router and the header explicitly states it should not be modified. The new
/insightsroute wiring is correctly generated and follows the existing patterns.packages/db/src/services/insights/types.ts (2)
71-92: Well-designed module interface with clear responsibilities.The
InsightModuleinterface cleanly separates concerns:
enumerateDimensions/computeManyfor data fetchingrenderfor presentation (with explicit "no DB reads" constraint)scorefor rankingdriversfor AI/explain featuresThe optional hooks and per-module threshold overrides provide good flexibility.
129-191: InsightStore abstraction provides clean persistence layer.The interface properly abstracts:
- Project enumeration for batch processing
- Identity-based lookups with composite key
- Upsert with material change tracking
- Event logging for audit trail
- Lifecycle management (close stale, suppress low-impact)
This enables easy testing with mock implementations and keeps the engine database-agnostic.
| const availableMetrics = Object.entries(payload?.metrics ?? {}); | ||
|
|
||
| // Pick what to display: prefer share if available (geo/devices), else primaryMetric | ||
| const [metricIndex, setMetricIndex] = useState( | ||
| availableMetrics.findIndex(([key]) => key === payload?.primaryMetric), | ||
| ); | ||
| const currentMetricKey = availableMetrics[metricIndex][0]; | ||
| const currentMetricEntry = availableMetrics[metricIndex][1]; |
There was a problem hiding this comment.
Add bounds checking for metrics array access.
The code doesn't handle the case where availableMetrics is empty or when findIndex returns -1. This will cause runtime errors when accessing availableMetrics[metricIndex] if no metrics are available or the primary metric is not found.
🔎 Apply this diff to add proper bounds checking:
const availableMetrics = Object.entries(payload?.metrics ?? {});
+ // Guard against empty metrics or missing primary metric
+ if (availableMetrics.length === 0) {
+ return null; // or render a fallback UI
+ }
+
// Pick what to display: prefer share if available (geo/devices), else primaryMetric
const [metricIndex, setMetricIndex] = useState(
availableMetrics.findIndex(([key]) => key === payload?.primaryMetric),
);
+
+ // Fallback to first metric if primary not found
+ const safeMetricIndex = metricIndex === -1 ? 0 : metricIndex;
- const currentMetricKey = availableMetrics[metricIndex][0];
- const currentMetricEntry = availableMetrics[metricIndex][1];
+ const currentMetricKey = availableMetrics[safeMetricIndex][0];
+ const currentMetricEntry = availableMetrics[safeMetricIndex][1];📝 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.
| const availableMetrics = Object.entries(payload?.metrics ?? {}); | |
| // Pick what to display: prefer share if available (geo/devices), else primaryMetric | |
| const [metricIndex, setMetricIndex] = useState( | |
| availableMetrics.findIndex(([key]) => key === payload?.primaryMetric), | |
| ); | |
| const currentMetricKey = availableMetrics[metricIndex][0]; | |
| const currentMetricEntry = availableMetrics[metricIndex][1]; | |
| const availableMetrics = Object.entries(payload?.metrics ?? {}); | |
| // Guard against empty metrics or missing primary metric | |
| if (availableMetrics.length === 0) { | |
| return null; // or render a fallback UI | |
| } | |
| // Pick what to display: prefer share if available (geo/devices), else primaryMetric | |
| const [metricIndex, setMetricIndex] = useState( | |
| availableMetrics.findIndex(([key]) => key === payload?.primaryMetric), | |
| ); | |
| // Fallback to first metric if primary not found | |
| const safeMetricIndex = metricIndex === -1 ? 0 : metricIndex; | |
| const currentMetricKey = availableMetrics[safeMetricIndex][0]; | |
| const currentMetricEntry = availableMetrics[safeMetricIndex][1]; |
🤖 Prompt for AI Agents
In apps/start/src/components/insights/insight-card.tsx around lines 36 to 43,
the code assumes availableMetrics has items and that findIndex finds the primary
metric — add bounds checking so we never index out of range: initialize
metricIndex to 0 when availableMetrics is empty or when findIndex returns -1 (or
set state to -1 and handle that case), and when computing
currentMetricKey/currentMetricEntry only access availableMetrics[metricIndex]
after confirming metricIndex is inside [0, availableMetrics.length-1]; otherwise
set currentMetricKey/currentMetricEntry to undefined (or a safe fallback) so
rendering logic can handle the "no metrics" case.
| {availableMetrics.length > 1 ? ( | ||
| <button | ||
| type="button" | ||
| className="text-[11px] text-muted-foreground capitalize flex items-center gap-1" | ||
| onClick={() => | ||
| setMetricIndex((metricIndex + 1) % availableMetrics.length) | ||
| } | ||
| > | ||
| <RotateCcwIcon className="size-2" /> | ||
| Show{' '} | ||
| {metricKeyToLabel( | ||
| availableMetrics[ | ||
| (metricIndex + 1) % availableMetrics.length | ||
| ][0], | ||
| )} | ||
| </button> |
There was a problem hiding this comment.
Division by zero risk in metric rotation.
The metric rotation logic uses % availableMetrics.length which would result in NaN if the array is empty. This is already addressed by the guard suggested in the previous comment, but ensure the fix is applied consistently.
🤖 Prompt for AI Agents
In apps/start/src/components/insights/insight-card.tsx around lines 135 to 150,
the metric rotation uses (metricIndex + 1) % availableMetrics.length which can
divide by zero if the array is empty; ensure you guard the calculation by
checking availableMetrics.length > 0 before computing the new index and only
call setMetricIndex when length > 0 (or length > 1 for rotation), and keep the
existing render guard consistent by disabling or hiding the button when
availableMetrics.length <= 1 so the modulo is never executed with zero.
apps/start/src/routes/_app.$organizationId.$projectId.insights.tsx
Outdated
Show resolved
Hide resolved
| /* | ||
| Warnings: | ||
| - Made the column `payload` on table `project_insights` required. This step will fail if there are existing NULL values in that column. | ||
| */ | ||
| -- AlterTable | ||
| ALTER TABLE "public"."project_insights" ALTER COLUMN "payload" SET NOT NULL, | ||
| ALTER COLUMN "payload" SET DEFAULT '{}'; |
There was a problem hiding this comment.
This migration will fail on any table containing NULL payload values.
The migration attempts to set a NOT NULL constraint without first backfilling existing NULL rows. While it includes SET DEFAULT '{}', this only applies to future inserts—not existing rows. SET NOT NULL may only be applied to a column provided none of the records in the table contain a NULL value for the column.
Add an UPDATE statement before the ALTER TABLE to backfill NULL values:
UPDATE "public"."project_insights" SET "payload" = '{}' WHERE "payload" IS NULL;Then apply the ALTER TABLE statement.
🤖 Prompt for AI Agents
In
packages/db/prisma/migrations/20251217204808_insight_payload_default/migration.sql
around lines 1 to 9, the migration sets payload to NOT NULL but does not
backfill existing NULL values, causing the ALTER TABLE to fail; add a preceding
UPDATE that sets payload = '{}' for rows where payload IS NULL, then run the
ALTER TABLE to set DEFAULT '{}' and SET NOT NULL so existing NULLs are fixed
before the constraint is applied.
| this._select | ||
| // Important: Expressions are treated as raw SQL; do not run escapeDate() | ||
| // on them, otherwise any embedded date strings get double-escaped | ||
| // (e.g. ''2025-12-16 23:59:59'') which ClickHouse rejects. | ||
| .map((col) => | ||
| col instanceof Expression ? col.toString() : this.escapeDate(col), | ||
| ) | ||
| .join(', '), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all usages of clix.exp() with context
rg -nP --type=ts --type=tsx --type=js --type=jsx -C3 'clix\.exp\s*\(' -g '!**/node_modules/**' -g '!**/dist/**' -g '!**/build/**'Repository: Openpanel-dev/openpanel
Length of output: 94
🏁 Script executed:
#!/bin/bash
# Find all usages of clix.exp() with context
rg -nP --type=ts --type=js -C3 'clix\.exp\s*\('Repository: Openpanel-dev/openpanel
Length of output: 19171
🏁 Script executed:
#!/bin/bash
# Read conversion.service.ts around line 98 to understand the group variable context
sed -n '80,120p' packages/db/src/services/conversion.service.tsRepository: Openpanel-dev/openpanel
Length of output: 1554
🏁 Script executed:
#!/bin/bash
# Find the function signature and beginning context for this code
sed -n '1,100p' packages/db/src/services/conversion.service.ts | head -80Repository: Openpanel-dev/openpanel
Length of output: 2523
🏁 Script executed:
#!/bin/bash
# Read event.service.ts around line 864 to see the nested exp() calls
sed -n '850,880p' packages/db/src/services/event.service.tsRepository: Openpanel-dev/openpanel
Length of output: 991
🏁 Script executed:
#!/bin/bash
# Find and read the formatClickhouseDate function
rg -n "formatClickhouseDate" packages/db/src -A 5 -B 2 | head -50Repository: Openpanel-dev/openpanel
Length of output: 4598
🏁 Script executed:
#!/bin/bash
# Find the formatClickhouseDate function definition
rg -n "formatClickhouseDate\s*=" packages/db/src/clickhouse/client.ts -A 3Repository: Openpanel-dev/openpanel
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Search for formatClickhouseDate function definition in clickhouse/client.ts
rg -n "formatClickhouseDate" packages/db/src/clickhouse/client.ts -B 2 -A 5Repository: Openpanel-dev/openpanel
Length of output: 268
🏁 Script executed:
#!/bin/bash
# Read the complete formatClickhouseDate function
sed -n '281,295p' packages/db/src/clickhouse/client.tsRepository: Openpanel-dev/openpanel
Length of output: 472
🏁 Script executed:
#!/bin/bash
# Check what getSelectPropertyKey does with the breakdown names
rg -n "getSelectPropertyKey" packages/db/src/services/chart.service.ts -B 2 -A 8 | head -40Repository: Openpanel-dev/openpanel
Length of output: 1466
🏁 Script executed:
#!/bin/bash
# Check how breakdowns flow into the query - trace back to IChartInput
rg -n "type IChartInput" packages -A 20 | head -50Repository: Openpanel-dev/openpanel
Length of output: 2108
🏁 Script executed:
#!/bin/bash
# Find the zChartInput schema definition to see how breakdowns are validated
rg -n "const zChartInput" packages/validation/src -A 30 | head -60Repository: Openpanel-dev/openpanel
Length of output: 4071
🏁 Script executed:
#!/bin/bash
# Find zChartBreakdowns and zChartBreakdown definitions
rg -n "const zChartBreakdown" packages/validation/src -B 2 -A 10 | head -60Repository: Openpanel-dev/openpanel
Length of output: 1927
🏁 Script executed:
#!/bin/bash
# Check the query-builder.ts file to see the clix.exp export and understand the Expression class
grep -n "export.*exp\|class Expression" packages/db/src/clickhouse/query-builder.ts | head -20Repository: Openpanel-dev/openpanel
Length of output: 88
🏁 Script executed:
#!/bin/bash
# Read the Expression class and clix.exp export
sed -n '37,80p' packages/db/src/clickhouse/query-builder.tsRepository: Openpanel-dev/openpanel
Length of output: 1241
🏁 Script executed:
#!/bin/bash
# Search for clix.exp export
rg -n "clix.exp\s*=" packages/db/src/clickhouse/query-builder.ts -B 2 -A 2Repository: Openpanel-dev/openpanel
Length of output: 251
The original concern about SQL injection via clix.exp() is valid and requires immediate attention.
While most clix.exp() usages are safe (using hardcoded functions like now(), toIntervalMonth(), or formatClickhouseDate()), there is a critical vulnerability in packages/db/src/services/conversion.service.ts around line 98.
The breakdownGroupBy array—derived from user-supplied breakdown names—is interpolated directly into the template literal passed to clix.exp():
const breakdownGroupBy = breakdowns.map((b, index) => `b_${index}`);
// Later in line 98-113:
clix.exp(`
(SELECT
a.${group},
...
${breakdownGroupBy.length ? `${breakdownGroupBy.join(', ')},` : ''}
...
GROUP BY a.${group}, a.a_time, a.event_day${breakdownGroupBy.length ? `, ${breakdownGroupBy.join(', ')}` : ''})
`)Although breakdownGroupBy values are aliased (e.g., b_0), the underlying getSelectPropertyKey(b.name) derives SQL expressions from user input without escaping, creating an injection vector. Validate and sanitize all user-controlled input before passing to clix.exp(), or refactor to use parameterized queries instead of template interpolation.
🤖 Prompt for AI Agents
In packages/db/src/clickhouse/query-builder.ts around lines 377 to 384, the
current code treats Expression inputs as raw SQL and the review points out a SQL
injection risk where user-derived breakdown expressions are interpolated into
clix.exp() templates; fix this by ensuring any value coming from
getSelectPropertyKey(b.name) is validated and escaped as an identifier (or
mapped only from a whitelist of allowed column/expression keys) before being
included in template literals, avoid raw string interpolation of user-controlled
expressions by constructing clix Expression objects or using
parameterized/escaped identifier helpers provided by the query builder, and keep
the aliased names (b_0, b_1...) derived only from safe, internal logic so the
final template only inserts validated/escaped identifiers or prebuilt Expression
instances.
| .select<{ cur_total: number }>([ | ||
| ctx.clix.exp( | ||
| `countIf(created_at BETWEEN '${formatClickhouseDate(ctx.window.start)}' AND '${formatClickhouseDate(getEndOfDay(ctx.window.end))}') as cur_total`, | ||
| ), | ||
| ]) | ||
| .from(TABLE_NAMES.sessions) | ||
| .where('project_id', '=', ctx.projectId) | ||
| .where('sign', '=', 1) | ||
| .where('created_at', 'BETWEEN', [ | ||
| ctx.window.baselineStart, | ||
| getEndOfDay(ctx.window.end), | ||
| ]) | ||
| .execute(), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for formatClickhouseDate implementation
rg -n "formatClickhouseDate" packages/db/src -A 5 -B 2 --type ts --type tsxRepository: Openpanel-dev/openpanel
Length of output: 94
🏁 Script executed:
#!/bin/bash
# Check the actual code at lines 56-68 in devices.module.ts
cat -n packages/db/src/services/insights/modules/devices.module.ts | sed -n '50,75p'
echo "---"
# Check if query builder is used in this file
rg -n "query-builder\|query-functions" packages/db/src/services/insights/modules/devices.module.ts -B 2 -A 2Repository: Openpanel-dev/openpanel
Length of output: 1157
🏁 Script executed:
#!/bin/bash
# Search for formatClickhouseDate with correct file type
rg -n "formatClickhouseDate" packages/db/src -A 5 -B 2 -t ts
echo "---"
# Check for the function definition
rg -n "export.*formatClickhouseDate\|function formatClickhouseDate" packages/db/src -A 10 -t tsRepository: Openpanel-dev/openpanel
Length of output: 28963
🏁 Script executed:
#!/bin/bash
# Check imports in the devices.module.ts file
head -30 packages/db/src/services/insights/modules/devices.module.tsRepository: Openpanel-dev/openpanel
Length of output: 903
🏁 Script executed:
#!/bin/bash
# Get the full formatClickhouseDate implementation
sed -n '281,295p' packages/db/src/clickhouse/client.tsRepository: Openpanel-dev/openpanel
Length of output: 472
🏁 Script executed:
#!/bin/bash
# Check query-functions.ts to see if there's a proper way to build this expression
head -80 packages/db/src/clickhouse/query-functions.tsRepository: Openpanel-dev/openpanel
Length of output: 171
🏁 Script executed:
#!/bin/bash
# Check what's in the clickhouse directory
ls -la packages/db/src/clickhouse/
echo "---"
# Check query-builder.ts
head -100 packages/db/src/clickhouse/query-builder.tsRepository: Openpanel-dev/openpanel
Length of output: 2876
🏁 Script executed:
#!/bin/bash
# Search for the exp method in query-builder.ts
rg -n "exp\(" packages/db/src/clickhouse/query-builder.ts -A 5 -B 2Repository: Openpanel-dev/openpanel
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Search for exp method definition
rg -n "\.exp" packages/db/src/clickhouse/query-builder.ts -A 3 -B 2
echo "---"
# Check if exp is part of the clix object
rg -n "exp" packages/db/src/clickhouse/query-builder.tsRepository: Openpanel-dev/openpanel
Length of output: 783
🏁 Script executed:
#!/bin/bash
# Check the clix.date function and other utility functions
sed -n '663,700p' packages/db/src/clickhouse/query-builder.ts
echo "---"
# Check if there are more utility functions
rg -n "^clix\." packages/db/src/clickhouse/query-builder.tsRepository: Openpanel-dev/openpanel
Length of output: 1720
Use query builder methods instead of raw SQL expressions.
Replace ctx.clix.exp() with template literals using clix.datetime() or similar utilities from the query builder. The code should construct queries through the query builder's designed interface rather than interpolating raw SQL strings, as established in the coding guidelines.
🤖 Prompt for AI Agents
In packages/db/src/services/insights/modules/devices.module.ts around lines 56
to 68, replace the raw SQL expression built with ctx.clix.exp(...) and
string-interpolated dates with equivalent query-builder calls: build the BETWEEN
date values using the clix datetime helper (e.g., ctx.clix.datetime or the
appropriate clix date helper) for the start and end, and construct the
countIf()/function call via the query-builder's function helpers (e.g.,
clix.func or clix.countIf) or by combining clix identifiers/columns and the clix
BETWEEN helper, passing the datetime values as parameters rather than embedding
them in a string; ensure the final select uses the query-builder expression
object so no raw SQL strings are interpolated.
| async enumerateDimensions(ctx) { | ||
| const { currentMap, baselineMap } = await fetchDeviceAggregates(ctx); | ||
| const topDims = selectTopDimensions( | ||
| currentMap, | ||
| baselineMap, | ||
| this.thresholds?.maxDims ?? 5, | ||
| ); | ||
| return topDims.map((dim) => `device:${dim}`); | ||
| }, | ||
|
|
||
| async computeMany(ctx, dimensionKeys): Promise<ComputeResult[]> { | ||
| const { currentMap, baselineMap, totalCurrent, totalBaseline } = | ||
| await fetchDeviceAggregates(ctx); |
There was a problem hiding this comment.
Double fetch of device aggregates degrades performance.
Both enumerateDimensions and computeMany call fetchDeviceAggregates, resulting in duplicate ClickHouse queries. Consider caching the result in the context or restructuring to fetch once.
🔎 Suggested approach
One pattern is to have enumerateDimensions store the aggregates in a shared context or return them alongside dimension keys, so computeMany can reuse them:
// Option 1: Cache in module-level or context
let cachedAggregates: Awaited<ReturnType<typeof fetchDeviceAggregates>> | null = null;
async function getDeviceAggregates(ctx: ComputeContext) {
if (!cachedAggregates) {
cachedAggregates = await fetchDeviceAggregates(ctx);
}
return cachedAggregates;
}
// Option 2: Pass data through engine workflow if supportedThis would cut the number of ClickHouse round-trips in half for this module.
🤖 Prompt for AI Agents
In packages/db/src/services/insights/modules/devices.module.ts around lines 178
to 190, both enumerateDimensions and computeMany independently call
fetchDeviceAggregates causing duplicate ClickHouse queries; modify the module so
fetchDeviceAggregates is executed once and its result reused — either cache the
returned aggregates on the provided ctx (e.g., ctx.deviceAggregates) the first
time it’s called and have both methods read from that cache, or change
enumerateDimensions to return the aggregates along with dimension keys and have
computeMany accept/use those aggregates; ensure the cached value includes
currentMap, baselineMap, totalCurrent and totalBaseline and that the cache is
invalidated or scoped appropriately to avoid cross-request leakage.
| ctx.clix.exp( | ||
| `countIf(created_at BETWEEN '${formatClickhouseDate(ctx.window.start)}' AND '${formatClickhouseDate(getEndOfDay(ctx.window.end))}') as cur_total`, | ||
| ), | ||
| ]) | ||
| .from(TABLE_NAMES.sessions) | ||
| .where('project_id', '=', ctx.projectId) | ||
| .where('sign', '=', 1) | ||
| .where('created_at', 'BETWEEN', [ | ||
| ctx.window.baselineStart, | ||
| getEndOfDay(ctx.window.end), | ||
| ]) | ||
| .execute(), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find formatClickhouseDate implementation
rg -n "formatClickhouseDate" --type ts -B 2 -A 5 packages/db/src
# Also check where it's imported from
rg -n "import.*formatClickhouseDate" --type ts packages/db/srcRepository: Openpanel-dev/openpanel
Length of output: 30006
🏁 Script executed:
#!/bin/bash
# Get context of geo.module.ts around lines 61-72
cat -n packages/db/src/services/insights/modules/geo.module.ts | sed -n '50,80p'Repository: Openpanel-dev/openpanel
Length of output: 1266
🏁 Script executed:
#!/bin/bash
# Check what ctx.clix is and if it aligns with coding guidelines
rg -n "ctx\.clix" --type ts packages/db/src/services/insights/modules/geo.module.ts -B 2 -A 2 | head -30
# Also check the query builder imports in this file
head -30 packages/db/src/services/insights/modules/geo.module.tsRepository: Openpanel-dev/openpanel
Length of output: 1823
🏁 Script executed:
#!/bin/bash
# Get complete formatClickhouseDate implementation
sed -n '281,295p' packages/db/src/clickhouse/client.tsRepository: Openpanel-dev/openpanel
Length of output: 472
🏁 Script executed:
#!/bin/bash
# Check the query-builder to understand clix and exp() method
head -100 packages/db/src/clickhouse/query-builder.ts | cat -nRepository: Openpanel-dev/openpanel
Length of output: 3211
🏁 Script executed:
#!/bin/bash
# Search for the exp method definition in query builder
rg -n "exp\(" packages/db/src/clickhouse/query-builder.ts -B 2 -A 5Repository: Openpanel-dev/openpanel
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Search for exp method more broadly
rg -n "\.exp" packages/db/src/clickhouse/query-builder.ts -B 2 -A 3Repository: Openpanel-dev/openpanel
Length of output: 388
🏁 Script executed:
#!/bin/bash
# Look for where clix is defined and exported
rg -n "export.*clix\|clix\s*=" packages/db/src/clickhouse/query-builder.ts -B 2 -A 3 | head -50Repository: Openpanel-dev/openpanel
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Check the query-functions.ts file to see what functions are available
head -50 packages/db/src/clickhouse/query-functions.tsRepository: Openpanel-dev/openpanel
Length of output: 171
🏁 Script executed:
#!/bin/bash
# Verify that clix is exported from query-builder
rg -n "export.*clix|^const clix|^export const clix" packages/db/src/clickhouse/query-builder.tsRepository: Openpanel-dev/openpanel
Length of output: 145
🏁 Script executed:
#!/bin/bash
# Check what files exist in the clickhouse directory
ls -la packages/db/src/clickhouse/Repository: Openpanel-dev/openpanel
Length of output: 427
🏁 Script executed:
#!/bin/bash
# Search for query-functions references in the codebase
rg -r "query-functions" packages/db/src/Repository: Openpanel-dev/openpanel
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Check if there are any parameterized query examples or safer patterns used elsewhere
rg -n "\.where.*BETWEEN.*\[" packages/db/src/services/insights/modules/geo.module.ts -B 2 -A 2Repository: Openpanel-dev/openpanel
Length of output: 1128
🏁 Script executed:
#!/bin/bash
# Check how the .where() method handles the BETWEEN operator with parameters
rg -n "where.*:" packages/db/src/clickhouse/query-builder.ts -A 20 | head -80Repository: Openpanel-dev/openpanel
Length of output: 2830
🏁 Script executed:
#!/bin/bash
# Get the full where method implementation
sed -n '150,250p' packages/db/src/clickhouse/query-builder.tsRepository: Openpanel-dev/openpanel
Length of output: 3034
🏁 Script executed:
#!/bin/bash
# Find the escapeValue method implementation
rg -n "escapeValue" packages/db/src/clickhouse/query-builder.ts -A 15 | head -50Repository: Openpanel-dev/openpanel
Length of output: 1797
🏁 Script executed:
#!/bin/bash
# Find the escapeDate method implementation
rg -n "escapeDate" packages/db/src/clickhouse/query-builder.ts -A 10Repository: Openpanel-dev/openpanel
Length of output: 2184
🏁 Script executed:
#!/bin/bash
# Check what clix.datetime returns
rg -n "clix\.datetime\|datetime.*=" packages/db/src/clickhouse/query-builder.ts -A 3Repository: Openpanel-dev/openpanel
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Look at all uses of ctx.clix.exp in geo.module.ts to see the full pattern
cat -n packages/db/src/services/insights/modules/geo.module.ts | sed -n '60,72p;106,111p;125,130p'Repository: Openpanel-dev/openpanel
Length of output: 1215
🏁 Script executed:
#!/bin/bash
# Verify the full context - check what ctx is and confirm it's using clix properly
rg -n "ctx\.clix\|ctx:" packages/db/src/services/insights/modules/geo.module.ts | head -20Repository: Openpanel-dev/openpanel
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Check if there's a different approach used in other files - look for safer patterns
rg -n "\.select.*countIf" packages/db/src/services/insights/modules/ -A 2 | head -40Repository: Openpanel-dev/openpanel
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Get broader context around the geo.module.ts code
sed -n '1,30p' packages/db/src/services/insights/modules/geo.module.tsRepository: Openpanel-dev/openpanel
Length of output: 867
🏁 Script executed:
#!/bin/bash
# Check what ctx type is
rg -n "ComputeContext\|ctx:" packages/db/src/services/insights/modules/geo.module.ts | head -10Repository: Openpanel-dev/openpanel
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Check if there's any guidance in the types file
find packages/db/src/services/insights -name "types*" -o -name "*types*"Repository: Openpanel-dev/openpanel
Length of output: 109
🏁 Script executed:
#!/bin/bash
# Check the ComputeContext type definition
rg -n "ComputeContext\|clix" packages/db/src/services/insights/types.ts -B 2 -A 5 | head -50Repository: Openpanel-dev/openpanel
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Read the types file directly
cat -n packages/db/src/services/insights/types.ts | head -100Repository: Openpanel-dev/openpanel
Length of output: 3929
🏁 Script executed:
#!/bin/bash
# Check if there are safe alternatives in the query builder for aggregate functions
rg -n "countIf\|aggregate\|expr" packages/db/src/clickhouse/query-builder.ts | head -20Repository: Openpanel-dev/openpanel
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Verify the full scope of the issue - check other modules for comparison
ls -la packages/db/src/services/insights/modules/Repository: Openpanel-dev/openpanel
Length of output: 577
🏁 Script executed:
#!/bin/bash
# Check how page-trends.module uses the same pattern to confirm consistency
diff -u <(sed -n '60,72p' packages/db/src/services/insights/modules/page-trends.module.ts) <(sed -n '60,72p' packages/db/src/services/insights/modules/geo.module.ts) || trueRepository: Openpanel-dev/openpanel
Length of output: 154
Use parameterized expressions consistently with the query builder's escapeDate method.
The raw string interpolation in ctx.clix.exp() bypasses the query builder's safe value escaping. While formatClickhouseDate produces safely formatted strings, this pattern is inconsistent with the parameterized .where() calls on the same query (lines 68–71), which properly use escapeValue(). Extract the formatted dates and pass them through the query builder's expression construction to maintain consistency with the parameterized approach used elsewhere in the builder API.
| async listProjectIdsForCadence(cadence: Cadence): Promise<string[]> { | ||
| const projects = await db.project.findMany({ | ||
| where: { | ||
| deleteAt: null, | ||
| eventsCount: { gt: 10_000 }, | ||
| updatedAt: { gt: new Date(Date.now() - 1000 * 60 * 60 * 24) }, | ||
| organization: { | ||
| subscriptionStatus: 'active', | ||
| }, | ||
| }, | ||
| select: { id: true }, | ||
| }); | ||
| return projects.map((p) => p.id); | ||
| }, |
There was a problem hiding this comment.
Unused cadence parameter in listProjectIdsForCadence.
The cadence parameter is declared but never used in the query logic. Either remove it or implement cadence-specific filtering if intended.
🔎 Options
If cadence should affect the query:
async listProjectIdsForCadence(cadence: Cadence): Promise<string[]> {
// Add cadence-specific filtering logic
const minEvents = cadence === 'daily' ? 10_000 : 50_000;
// ...
}If not needed:
async listProjectIdsForCadence(_cadence: Cadence): Promise<string[]> {
// Prefix with underscore to indicate intentionally unused🤖 Prompt for AI Agents
packages/db/src/services/insights/store.ts lines 12-25: the cadence parameter is
declared but not used; either remove it, rename it to _cadence to signal it's
intentionally unused, or implement cadence-specific filtering (e.g., compute a
minEvents or updatedAt threshold based on cadence and apply it in the query) —
update the function signature and body accordingly and run type checks to ensure
no unused-parameter lint errors remain.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
apps/start/src/routes/_app.$organizationId.$projectId.insights.tsx (4)
52-63: Minor:replaceonly replaces the first hyphen occurrence.The fallback
moduleKey.replace('-', ' ')only replaces the first hyphen. If a module key ever has multiple hyphens (e.g.,my-new-module), it would display asmy new-module.🔎 Consider using replaceAll for consistency
- return displayNames[moduleKey] || moduleKey.replace('-', ' '); + return displayNames[moduleKey] || moduleKey.replaceAll('-', ' ');
164-199: Consider extracting duplicatedseverityOrderconstant.The
severityOrdermapping is duplicated in bothseverity-descandseverity-ascswitch cases. This can be extracted to avoid duplication.🔎 Extract severityOrder outside the sort function
+const SEVERITY_ORDER: Record<string, number> = { + severe: 3, + moderate: 2, + low: 1, +}; + const filteredAndSorted = useMemo(() => { // ... filtering logic ... const sorted = [...filtered].sort((a, b) => { switch (sortBy) { // ... other cases ... case 'severity-desc': { - const severityOrder: Record<string, number> = { - severe: 3, - moderate: 2, - low: 1, - }; - const aSev = severityOrder[a.severityBand ?? ''] ?? 0; - const bSev = severityOrder[b.severityBand ?? ''] ?? 0; + const aSev = SEVERITY_ORDER[a.severityBand ?? ''] ?? 0; + const bSev = SEVERITY_ORDER[b.severityBand ?? ''] ?? 0; return bSev - aSev; } case 'severity-asc': { - const severityOrder: Record<string, number> = { - severe: 3, - moderate: 2, - low: 1, - }; - const aSev = severityOrder[a.severityBand ?? ''] ?? 0; - const bSev = severityOrder[b.severityBand ?? ''] ?? 0; + const aSev = SEVERITY_ORDER[a.severityBand ?? ''] ?? 0; + const bSev = SEVERITY_ORDER[b.severityBand ?? ''] ?? 0; return aSev - bSev; } // ... } }); }, [/* deps */]);
382-412: Remove unusedindexparameter.The
indexparameter on line 382 is declared but never used in the map callback.🔎 Remove unused parameter
- {moduleInsights.map((insight, index) => ( + {moduleInsights.map((insight) => (
391-410: Consider simplifying theonFiltercallback creation.The IIFE pattern works but is verbose. A simpler approach could use a conditional directly.
🔎 Simplified callback
<InsightCard insight={insight} - onFilter={(() => { - const filterString = insight.payload?.dimensions - .map( - (dim) => - `${dim.key},is,${encodeURIComponent(dim.value)}`, - ) - .join(';'); - if (filterString) { - return () => { - navigate({ - to: '/$organizationId/$projectId', - from: Route.fullPath, - search: { - f: filterString, - }, - }); - }; - } - return undefined; - })()} + onFilter={ + insight.payload?.dimensions?.length + ? () => { + const filterString = insight.payload.dimensions + .map( + (dim) => + `${dim.key},is,${encodeURIComponent(dim.value)}`, + ) + .join(';'); + navigate({ + to: '/$organizationId/$projectId', + from: Route.fullPath, + search: { f: filterString }, + }); + } + : undefined + } />
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/start/src/routes/_app.$organizationId.$projectId.insights.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursorrules)
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
Files:
apps/start/src/routes/_app.$organizationId.$projectId.insights.tsx
apps/start/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/start/.cursorrules)
Instrument server functions with Sentry using
Sentry.startSpan()to wrap the implementation. Import Sentry with:import * as Sentry from '@sentry/tanstackstart-react'
Files:
apps/start/src/routes/_app.$organizationId.$projectId.insights.tsx
🧠 Learnings (4)
📚 Learning: 2025-12-15T09:20:11.883Z
Learnt from: CR
Repo: Openpanel-dev/openpanel PR: 0
File: apps/start/.cursorrules:0-0
Timestamp: 2025-12-15T09:20:11.883Z
Learning: Applies to apps/start/routes/**/*.{ts,tsx} : Un-nest routes by suffixing a segment with `_` (e.g., `posts_.$postId.edit.tsx`) to render independently from parent routes
Applied to files:
apps/start/src/routes/_app.$organizationId.$projectId.insights.tsx
📚 Learning: 2025-12-15T09:20:11.883Z
Learnt from: CR
Repo: Openpanel-dev/openpanel PR: 0
File: apps/start/.cursorrules:0-0
Timestamp: 2025-12-15T09:20:11.883Z
Learning: Applies to apps/start/routes/**/*.{ts,tsx} : Define routes using `createFileRoute(path)` with the path auto-managed by the bundler/CLI
Applied to files:
apps/start/src/routes/_app.$organizationId.$projectId.insights.tsx
📚 Learning: 2025-12-15T09:20:11.883Z
Learnt from: CR
Repo: Openpanel-dev/openpanel PR: 0
File: apps/start/.cursorrules:0-0
Timestamp: 2025-12-15T09:20:11.883Z
Learning: Applies to apps/start/routes/**/*.{ts,tsx} : Use `<Outlet/>` component in layout routes to render child route components
Applied to files:
apps/start/src/routes/_app.$organizationId.$projectId.insights.tsx
📚 Learning: 2025-12-15T09:20:11.883Z
Learnt from: CR
Repo: Openpanel-dev/openpanel PR: 0
File: apps/start/.cursorrules:0-0
Timestamp: 2025-12-15T09:20:11.883Z
Learning: Applies to apps/start/routes/**/*.{ts,tsx} : Use `(group)` folder names to organize routes for file structure only, without affecting URL paths or nesting
Applied to files:
apps/start/src/routes/_app.$organizationId.$projectId.insights.tsx
🧬 Code graph analysis (1)
apps/start/src/routes/_app.$organizationId.$projectId.insights.tsx (4)
apps/start/src/utils/title.ts (2)
createProjectTitle(39-53)PAGE_TITLES(78-129)apps/start/src/components/page-header.tsx (1)
PageHeader(11-30)apps/start/src/components/ui/table.tsx (1)
TableButtons(5-17)apps/start/src/components/insights/insight-card.tsx (1)
InsightCard(29-199)
🔇 Additional comments (6)
apps/start/src/routes/_app.$organizationId.$projectId.insights.tsx (6)
1-28: LGTM!Imports are well-organized and appropriate for the component's functionality. The file is a client route component, so the Sentry instrumentation guideline for server functions doesn't apply here.
30-43: LGTM!Route definition follows the established pattern using
createFileRoutewith proper path parameters. The head meta configuration correctly usescreateProjectTitlefor SEO.
65-74: LGTM with a note on the hardcoded limit.The data fetching pattern using
useQuerywith TRPC query options is correct. The hardcodedlimit: 500works for now but consider making it configurable or implementing pagination if the insights count grows significantly.
76-112: LGTM!Excellent use of
nuqsfor URL-synced filter state. This enables shareable and bookmarkable filter configurations. TheparseAsStringEnumusage provides good type safety for constrained values.
212-241: LGTM!The grouping logic correctly uses a
Mapfor efficient aggregation and implements clear sorting priority (referrers first, then by average impact). TheuseMemodependency is correct.
243-270: LGTM!Loading state provides appropriate skeleton placeholders that mirror the final UI structure, ensuring a smooth loading experience.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.