Feature/revenue 2#237
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdds end-to-end revenue tracking: DB schema and migrations, session-level revenue aggregation, SDK/web methods for revenue and device-id handling, API endpoint and validation, worker/session changes to carry revenue, overview/chart UI updates to surface revenue, docs and small frontend UX/responsive adjustments. Changes
Sequence Diagram(s)sequenceDiagram
participant SDK as Client SDK
participant Web as Web SDK
participant API as Track API
participant Redis as Redis (sessionBuffer)
participant Worker as Event Worker
participant DB as ClickHouse
Note over SDK,Web: Emitting revenue from client
SDK->>API: POST /track { name: 'revenue', __revenue, __deviceId?, ... }
Web->>Web: pendingRevenue() enqueues or flushRevenue() sends queued revenue
API->>API: validate revenue allowed (allowUnsafeRevenueTracking or client secret)
API->>Redis: fetch device/session keys (multi-exists / getExistingSession)
Redis-->>API: deviceId/session data
API->>Worker: enqueue revenue event
Worker->>Redis: sessionBuffer.getExistingSession({ sessionId } or by profile)
Redis-->>Worker: session record (includes revenue)
Worker->>Worker: aggregate/merge revenue into session
Worker->>DB: insert event record (includes revenue field)
DB-->>Worker: acknowledgement
sequenceDiagram
participant Frontend as Overview UI
participant tRPC as TRPC Router
participant Service as OverviewService
participant CH as ClickHouse
Frontend->>tRPC: getMetrics({ projectId, range })
tRPC->>Service: getMetrics(...)
Service->>CH: query metrics including total_revenue and series
CH-->>Service: metrics + total_revenue
Service-->>tRPC: enriched payload with total_revenue, prev_total_revenue
tRPC-->>Frontend: response
Frontend->>Frontend: render composed chart (value line + revenue bars) and revenue blocks
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used📓 Path-based instructions (1)apps/start/**/*.{ts,tsx}📄 CodeRabbit inference engine (apps/start/.cursorrules)
Files:
🧬 Code graph analysis (1)apps/start/src/components/overview/overview-metric-card.tsx (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (4)
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/start/src/components/overview/overview-metrics.tsx (1)
104-112: Remove leftover debug logging.This
console.log('data', data)runs on every render and dumps all metric values (including revenue) to the browser console. It’s noisy in production and leaks sensitive telemetry. Please drop it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
apps/api/src/controllers/profile.controller.ts(1 hunks)apps/api/src/routes/profile.router.ts(1 hunks)apps/public/components/flow-step.tsx(1 hunks)apps/public/content/docs/get-started/revenue-tracking.mdx(1 hunks)apps/start/src/components/charts/common-bar.tsx(1 hunks)apps/start/src/components/events/event-icon.tsx(1 hunks)apps/start/src/components/overview/overview-metric-card.tsx(2 hunks)apps/start/src/components/overview/overview-metrics.tsx(12 hunks)apps/worker/src/jobs/events.create-session-end.ts(5 hunks)apps/worker/src/jobs/events.incoming-event.ts(5 hunks)apps/worker/src/jobs/events.incoming-events.test.ts(1 hunks)packages/db/code-migrations/6-add-revenue-column.sql(1 hunks)packages/db/code-migrations/6-add-revenue-column.ts(1 hunks)packages/db/src/buffers/event-buffer.test.ts(0 hunks)packages/db/src/buffers/event-buffer.ts(1 hunks)packages/db/src/buffers/session-buffer.ts(5 hunks)packages/db/src/clickhouse/client.ts(1 hunks)packages/db/src/services/chart.service.ts(1 hunks)packages/db/src/services/event.service.ts(5 hunks)packages/db/src/services/overview.service.ts(12 hunks)packages/sdks/sdk/src/index.ts(1 hunks)packages/sdks/web/src/index.ts(3 hunks)packages/sdks/web/src/types.d.ts(1 hunks)packages/trpc/src/routers/chart.ts(1 hunks)packages/trpc/src/routers/overview.ts(2 hunks)
💤 Files with no reviewable changes (1)
- packages/db/src/buffers/event-buffer.test.ts
🧰 Additional context used
📓 Path-based instructions (1)
apps/start/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/start/.cursorrules)
apps/start/**/*.{ts,tsx}: Instrument server functions created with createServerFn by wrapping their implementation in Sentry.startSpan and importing Sentry from '@sentry/tanstackstart-react'
When using Sentry in the TanStack Start React app, import it as: import * as Sentry from '@sentry/tanstackstart-react'
Files:
apps/start/src/components/events/event-icon.tsxapps/start/src/components/charts/common-bar.tsxapps/start/src/components/overview/overview-metric-card.tsxapps/start/src/components/overview/overview-metrics.tsx
🧠 Learnings (1)
📚 Learning: 2025-09-26T18:30:17.476Z
Learnt from: CR
Repo: Openpanel-dev/openpanel PR: 0
File: .cursorrules:0-0
Timestamp: 2025-09-26T18:30:17.476Z
Learning: When writing ClickHouse queries, use the helper functions in ./packages/db/src/clickhouse/query-functions.ts rather than crafting raw queries
Applied to files:
packages/db/code-migrations/6-add-revenue-column.ts
🧬 Code graph analysis (8)
packages/db/src/services/overview.service.ts (1)
packages/db/src/clickhouse/query-builder.ts (1)
clix(650-652)
packages/db/src/services/event.service.ts (3)
packages/db/src/services/profile.service.ts (1)
IServiceProfile(216-240)packages/db/src/services/session.service.ts (1)
IClickhouseSession(17-57)packages/db/src/clickhouse/client.ts (1)
convertClickhouseDateToJs(293-295)
packages/sdks/web/src/index.ts (1)
packages/sdks/sdk/src/index.ts (1)
OpenPanel(73-212)
packages/db/code-migrations/6-add-revenue-column.ts (2)
packages/db/code-migrations/helpers.ts (1)
getIsCluster(17-24)packages/db/src/clickhouse/migration.ts (2)
addColumns(137-152)runClickhouseMigrationCommands(338-470)
apps/api/src/controllers/profile.controller.ts (3)
packages/db/src/services/salt.service.ts (1)
getSalts(20-49)apps/api/src/utils/get-client-ip.ts (1)
getClientIp(6-8)packages/redis/redis.ts (1)
getRedisCache(66-72)
apps/worker/src/jobs/events.incoming-event.ts (1)
packages/db/src/buffers/index.ts (1)
sessionBuffer(9-9)
apps/start/src/components/overview/overview-metrics.tsx (3)
packages/common/src/get-previous-metric.ts (1)
getPreviousMetric(7-39)apps/public/lib/utils.ts (1)
cn(4-6)apps/start/src/utils/theme.ts (1)
getChartColor(25-27)
apps/worker/src/jobs/events.create-session-end.ts (3)
packages/db/src/services/event.service.ts (3)
IServiceEvent(199-237)getEvents(313-356)transformSessionToEvent(104-151)packages/db/src/buffers/index.ts (1)
sessionBuffer(9-9)packages/common/src/date.ts (1)
getTime(5-7)
🪛 LanguageTool
apps/public/content/docs/get-started/revenue-tracking.mdx
[grammar] ~174-~174: Ensure spelling is correct
Context: ... work if your OpenPanel instance didn't recieve a client secret (for securuty reasons)....
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~174-~174: Ensure spelling is correct
Context: ...nce didn't recieve a client secret (for securuty reasons). You can enable frontend reven...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (28)
packages/db/src/clickhouse/client.ts (1)
29-31: Verify that silencing debug logs is intentional.The
logger.info(message, args)call in thedebugmethod is now commented out, which means ClickHouse client debug messages will no longer appear in logs. This may complicate troubleshooting in development or when diagnosing production issues.Can you confirm this is intentional to reduce log noise in production? If so, consider whether a conditional check (e.g., based on
NODE_ENV) would allow debug logs in development while keeping production clean.apps/start/src/components/events/event-icon.tsx (1)
51-54: LGTM!The revenue icon entry is properly configured with
DollarSignIcon(mapped at line 135) andgreencolor, following the same pattern as other event icons.apps/start/src/components/charts/common-bar.tsx (1)
80-88: LGTM!The
BarShapeGreendefinition follows the same pattern asBarShapeBlueandBarShapeGrey, with appropriate green color values and consistent opacity transitions for active states.apps/public/components/flow-step.tsx (1)
1-79: LGTM!The
FlowStepcomponent is well-structured with:
- Clear prop types and sensible defaults
- Proper icon and color mapping
- Conditional connector line rendering
- Good separation of concerns (layout, styling, content)
The component will serve well for visualizing flow sequences in the documentation.
apps/public/content/docs/get-started/revenue-tracking.mdx (1)
1-202: Excellent documentation structure.The revenue tracking documentation is comprehensive and well-organized:
- Clear explanation of the device_id linking mechanism
- Two distinct approaches (webhook vs frontend) with practical code examples
- Good use of FlowStep components for visual clarity
- Helpful pros/cons comparison
- Complete API reference for available methods
The Stripe integration examples will be especially helpful for users implementing revenue tracking.
apps/api/src/routes/profile.router.ts (1)
28-32: LGTM!The new GET route for profile info is properly configured and follows the same pattern as existing routes. Having both GET and POST on the root path is valid since they use different HTTP methods.
packages/db/code-migrations/6-add-revenue-column.sql (1)
1-1: LGTM!The migration is well-designed:
IF NOT EXISTSensures idempotencyUInt64is appropriate for revenue values (can handle large amounts in smallest currency units)CODEC(Delta(4), LZ4)provides efficient storage for numeric datapackages/sdks/sdk/src/index.ts (1)
186-191: Verify backend handling of__revenueproperty.The
revenuemethod tracks an event named 'revenue' with the amount passed as the__revenueproperty. This implementation is clean and delegates to the existingtrackmethod.Please confirm that:
- The backend expects the revenue amount in the
__revenueproperty- This aligns with the database migration that adds a
revenuecolumn (packages/db/code-migrations/6-add-revenue-column.sql)- The event service properly extracts
__revenuefrom properties and maps it to therevenuecolumnYou can verify this by checking the event transformation logic in the backend services.
packages/trpc/src/routers/chart.ts (1)
209-210: LGTM! Revenue property added to queryable properties.The addition of 'revenue' to the properties list allows frontend queries to filter and analyze revenue data alongside other event properties.
apps/start/src/components/overview/overview-metric-card.tsx (2)
25-25: LGTM! Currency unit type added.The new 'currency' unit option enables revenue metrics to be formatted as currency values.
60-63: LGTM! Currency formatting with cents-to-dollars conversion.The implementation correctly converts revenue from cents to dollars before formatting. This assumes revenue is consistently stored in cents throughout the system.
packages/trpc/src/routers/overview.ts (2)
237-237: LGTM! Previous revenue metric added to stats endpoint.The addition follows the established pattern for previous-period metrics, maintaining consistency with other comparative fields like
prev_bounce_rateandprev_unique_visitors.
250-250: LGTM! Previous revenue added to series data.Consistent with the metrics-level change, this enables time-series comparison of revenue data.
packages/db/src/services/chart.service.ts (4)
176-182: LGTM! Revenue-specific handling for property_sum.The specialized path for revenue correctly uses the native column with
revenue > 0filtering, which is more appropriate than the generic property extraction logic.
186-192: LGTM! Revenue-specific handling for property_average.Consistent with the property_sum implementation, this correctly aggregates revenue values from the native column.
196-202: LGTM! Revenue-specific handling for property_max.The pattern is correctly applied for maximum revenue calculations.
206-212: LGTM! Revenue-specific handling for property_min.The pattern is correctly applied for minimum revenue calculations, completing the set of aggregation operations.
packages/db/code-migrations/6-add-revenue-column.ts (3)
9-14: LGTM! Revenue column migration with appropriate data type and compression.The
UInt64type is suitable for storing revenue in cents, and theCODEC(Delta(4), LZ4)provides efficient compression for monetary values. Delta encoding is particularly effective for incremental numerical data.
16-27: LGTM! SQL file generation for migration tracking.Writing the migration SQL to a file provides useful documentation and debugging capabilities.
29-31: LGTM! Dry-run support for safe migration testing.The
--dryflag allows validation of the migration SQL without executing it, which is a good safety practice.apps/api/src/controllers/profile.controller.ts (4)
15-31: LGTM! Input validation for profile info endpoint.The function properly validates required inputs (projectId, IP address, and user-agent) and returns appropriate 400 errors when missing.
32-44: LGTM! Device ID generation with salt rotation support.Generating device IDs with both current and previous salts ensures continuity during salt rotation periods.
45-61: LGTM! Efficient Redis lookup with multi command.Using Redis multi to batch both EXISTS checks is efficient and correctly returns the matching device ID.
62-68: Empty deviceId fallback is safe; manually verify any external consumers.The
infoendpoint intentionally returns an empty string fordeviceIdwhen Redis operations fail or no session is found (lines 66-68). This is a safe fallback—returning an empty string is preferable tonullorundefinedand the inferred return type{ deviceId: string }explicitly allows it.No validation constraints or broken dependencies were found in the codebase. However, since external API consumers may exist outside the visible codebase, verify that any clients or SDKs calling this endpoint gracefully handle the empty
deviceIdresponse.packages/sdks/web/src/types.d.ts (1)
11-15: LGTM! Revenue tracking methods added to SDK public API.The additions expose four revenue-related methods (
revenue,flushRevenue,clearRevenue,pendingRevenue) to the web SDK's public interface, enabling client-side revenue tracking functionality.apps/worker/src/jobs/events.incoming-events.test.ts (1)
1-1: LGTM! Test imports updated to reflect session-based event processing.The removal of
IServiceEventimport aligns with the migration from lastScreenView-based logic to session-based event processing.packages/db/src/services/overview.service.ts (1)
150-327: Revenue totals wired correctly
The rollup-basedtotal_revenueplumbing mirrors the existing bounce/session patterns and cleanly surfaces both daily and aggregate values. Looks good.apps/worker/src/jobs/events.create-session-end.ts (1)
45-109: Session-derived last event flow looks solid
Pulling the session snapshot viatransformSessionToEventkeeps the notification stream ordered while ensuring the session_end payload reuses the authoritative session data. Nice cleanup.
lindesvard
force-pushed
the
feature/revenue-2
branch
from
79c3a66 to
15e476c
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/trpc/src/routers/overview.ts (1)
110-119: Restore visitor counts for anonymous traffic.Switching
visitor_counttouniq(profile_id)means any event without an identified profile now contributes zero visitors. Most traffic arrives before identification, so the realtime chart will collapse to 0 for anonymous sessions. Please revert to counting a stable identifier that exists for every session (e.g.,session_idorcoalesce(profile_id, device_id, session_id)), so anonymous visitors remain visible.packages/db/src/services/overview.service.ts (1)
208-227: Ensure filled series returns numeric revenue valuesRows produced by
WITH FILLdon’t have a matchingdaily_statsentry, sods.total_revenueremains the default for a nullable column — i.e.NULL. That leaves the series emittingnulleven though the public type promisesnumber, and downstream charting will see gaps instead of zeros. ClickHouse fills only the ordering column; every other column keeps its default value, and forNullable(...)that default isNULL. (docs-content.clickhouse.tech)Wrap the join column with
coalesce(orifNull) so gap rows report0, and update theGROUP BYlist to match the new expression:- 'ds.total_revenue AS total_revenue', + 'coalesce(ds.total_revenue, 0) AS total_revenue', - .groupBy(['date', 'ds.bounce_rate', 'ds.total_revenue']) + .groupBy([ + 'date', + 'ds.bounce_rate', + 'coalesce(ds.total_revenue, 0)', + ])
coalesceis built for exactly this nullable-to-default projection. (clickhouse.com)
♻️ Duplicate comments (4)
packages/sdks/web/src/index.ts (1)
43-47: Guard sessionStorage access to avoid crashing the SDK.This issue was previously flagged:
sessionStorage.getItemandJSON.parsecan throw in Safari private mode, when storage quota is exhausted, or with corrupted/legacy values (including the literal string"null"). Currently, any exception here prevents the entire SDK from initializing, breaking all tracking.Wrap the access in a try-catch block:
if (!this.isServer()) { console.log('OpenPanel.dev - Initialized', this.options); - const pending = sessionStorage.getItem('openpanel-pending-revenues'); - if (pending) { - this.pendingRevenues = JSON.parse(pending); - } + try { + const pending = sessionStorage.getItem('openpanel-pending-revenues'); + if (pending && pending !== 'null') { + this.pendingRevenues = JSON.parse(pending); + } + } catch (error) { + console.warn('OpenPanel.dev - Failed to load pending revenues', error); + this.pendingRevenues = []; + } this.setGlobalProperties({apps/public/content/docs/get-started/revenue-tracking.mdx (1)
330-330: Fix the remaining spelling mistakes.This sentence still contains the previously flagged typos (“recieve”, “securuty”). Please update it to use “receive” and “security”.
-The revenue method will create a revenue event. It's important to know that this method will not work if your OpenPanel instance didn't recieve a client secret (for securuty reasons). You can enable frontend revenue tracking within your project settings. +The revenue method will create a revenue event. It's important to know that this method will not work if your OpenPanel instance didn't receive a client secret (for security reasons). You can enable frontend revenue tracking within your project settings.apps/start/src/components/overview/overview-metrics.tsx (1)
111-112: Remove debug logging before shipping. Theseconsole.logcalls spam render-time data (including revenue) into production consoles again, contradicting the earlier review directive and leaking sensitive metrics. Drop both logs (console.log('data', data)andconsole.log('data', dotIndex)) before shipping.packages/db/src/buffers/session-buffer.ts (1)
95-96: Guard the revenue accumulator against undefined. Legacy sessions often haverevenueunset; using+=in that state turns the field intoNaN, corrupting sessions and downstream revenue sums. Initialize to0before adding.- newSession.revenue += event.name === 'revenue' ? (event.revenue ?? 0) : 0; + const revenueDelta = + event.name === 'revenue' ? (event.revenue ?? 0) : 0; + newSession.revenue = (newSession.revenue ?? 0) + revenueDelta;
🧹 Nitpick comments (2)
apps/start/src/components/events/table/item.tsx (1)
95-129: Consider displaying revenue data in the event metadata row.Since this PR introduces revenue tracking and
IServiceEventnow includes an optionalrevenuefield, you might want to display revenue alongside other event metadata (referrer, OS, browser, etc.) when present.For example, you could add a revenue Pill similar to the existing metadata pills:
{event.revenue != null && viewOptions.revenue !== false && ( <Pill icon={<SerieIcon name="revenue" />}> {formatCurrency(event.revenue)} </Pill> )}This would make revenue immediately visible in event lists, enhancing the revenue tracking feature's visibility.
packages/db/src/services/chart.service.ts (1)
176-214: Consider extracting the revenue property logic to reduce duplication.The revenue-specific handling is repeated across four aggregation types (sum, average, max, min). While functionally correct, this pattern could be refactored into a helper function to improve maintainability.
For example:
function getAggregationSelect( aggregation: 'sum' | 'avg' | 'max' | 'min', property: string ): { select: string; where: string } { if (property === 'revenue') { return { select: `${aggregation}(revenue) as count`, where: 'revenue > 0' }; } return { select: `${aggregation}(toFloat64(${getSelectPropertyKey(property)})) as count`, where: `${getSelectPropertyKey(property)} IS NOT NULL AND notEmpty(${getSelectPropertyKey(property)})` }; }Then use it in each segment block.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
apps/api/src/controllers/profile.controller.ts(1 hunks)apps/api/src/controllers/track.controller.ts(4 hunks)apps/api/src/routes/track.router.ts(2 hunks)apps/public/components/flow-step.tsx(1 hunks)apps/public/content/docs/get-started/revenue-tracking.mdx(1 hunks)apps/public/package.json(1 hunks)apps/start/src/components/charts/common-bar.tsx(1 hunks)apps/start/src/components/events/event-icon.tsx(1 hunks)apps/start/src/components/events/table/item.tsx(4 hunks)apps/start/src/components/overview/overview-live-histogram.tsx(1 hunks)apps/start/src/components/overview/overview-metric-card.tsx(2 hunks)apps/start/src/components/overview/overview-metrics.tsx(12 hunks)apps/start/src/components/overview/overview-widget-table.tsx(8 hunks)apps/start/src/components/realtime/realtime-live-histogram.tsx(1 hunks)apps/start/src/components/realtime/realtime-reloader.tsx(1 hunks)apps/start/src/components/widget-table.tsx(4 hunks)apps/start/src/hooks/use-numer-formatter.ts(1 hunks)apps/worker/src/jobs/events.create-session-end.ts(5 hunks)apps/worker/src/jobs/events.incoming-event.ts(5 hunks)apps/worker/src/jobs/events.incoming-events.test.ts(1 hunks)packages/db/code-migrations/6-add-revenue-column.sql(1 hunks)packages/db/code-migrations/6-add-revenue-column.ts(1 hunks)packages/db/src/buffers/event-buffer.test.ts(0 hunks)packages/db/src/buffers/event-buffer.ts(1 hunks)packages/db/src/buffers/session-buffer.ts(5 hunks)packages/db/src/clickhouse/client.ts(1 hunks)packages/db/src/services/chart.service.ts(1 hunks)packages/db/src/services/event.service.ts(5 hunks)packages/db/src/services/overview.service.ts(16 hunks)packages/sdks/sdk/src/index.ts(1 hunks)packages/sdks/web/src/index.ts(3 hunks)packages/sdks/web/src/types.d.ts(1 hunks)packages/trpc/src/routers/chart.ts(1 hunks)packages/trpc/src/routers/overview.ts(4 hunks)
💤 Files with no reviewable changes (1)
- packages/db/src/buffers/event-buffer.test.ts
✅ Files skipped from review due to trivial changes (1)
- apps/start/src/components/overview/overview-live-histogram.tsx
🚧 Files skipped from review as they are similar to previous changes (11)
- apps/start/src/components/overview/overview-metric-card.tsx
- packages/db/src/clickhouse/client.ts
- packages/sdks/sdk/src/index.ts
- apps/start/src/components/events/event-icon.tsx
- apps/worker/src/jobs/events.incoming-events.test.ts
- apps/worker/src/jobs/events.incoming-event.ts
- packages/sdks/web/src/types.d.ts
- packages/db/src/services/event.service.ts
- apps/public/components/flow-step.tsx
- packages/db/code-migrations/6-add-revenue-column.ts
- packages/trpc/src/routers/chart.ts
🧰 Additional context used
📓 Path-based instructions (1)
apps/start/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/start/.cursorrules)
apps/start/**/*.{ts,tsx}: Instrument server functions created with createServerFn by wrapping their implementation in Sentry.startSpan and importing Sentry from '@sentry/tanstackstart-react'
When using Sentry in the TanStack Start React app, import it as: import * as Sentry from '@sentry/tanstackstart-react'
Files:
apps/start/src/components/events/table/item.tsxapps/start/src/components/realtime/realtime-live-histogram.tsxapps/start/src/components/realtime/realtime-reloader.tsxapps/start/src/hooks/use-numer-formatter.tsapps/start/src/components/overview/overview-metrics.tsxapps/start/src/components/charts/common-bar.tsxapps/start/src/components/widget-table.tsxapps/start/src/components/overview/overview-widget-table.tsx
🧬 Code graph analysis (9)
apps/api/src/controllers/track.controller.ts (4)
packages/sdks/sdk/src/index.ts (1)
fetchDeviceId(199-206)packages/db/src/services/salt.service.ts (1)
getSalts(20-49)apps/api/src/utils/get-client-ip.ts (1)
getClientIp(6-8)packages/redis/redis.ts (1)
getRedisCache(66-72)
apps/start/src/components/realtime/realtime-live-histogram.tsx (3)
apps/start/src/components/charts/common-bar.tsx (1)
BarShapeBlue(71-79)apps/start/src/hooks/use-numer-formatter.ts (1)
useNumber(73-108)apps/start/src/utils/theme.ts (1)
getChartColor(25-27)
apps/worker/src/jobs/events.create-session-end.ts (3)
packages/db/src/services/event.service.ts (3)
IServiceEvent(199-237)getEvents(313-356)transformSessionToEvent(104-151)packages/db/src/buffers/index.ts (1)
sessionBuffer(9-9)packages/common/src/date.ts (1)
getTime(5-7)
apps/start/src/components/overview/overview-metrics.tsx (4)
packages/common/src/get-previous-metric.ts (1)
getPreviousMetric(7-39)apps/start/src/hooks/use-numer-formatter.ts (1)
useNumber(73-108)apps/public/lib/utils.ts (1)
cn(4-6)apps/start/src/utils/theme.ts (1)
getChartColor(25-27)
apps/api/src/routes/track.router.ts (2)
apps/api/src/controllers/track.controller.ts (1)
fetchDeviceId(410-467)packages/sdks/sdk/src/index.ts (1)
fetchDeviceId(199-206)
apps/start/src/components/widget-table.tsx (1)
apps/public/lib/utils.ts (1)
cn(4-6)
packages/db/src/services/overview.service.ts (1)
packages/db/src/clickhouse/query-builder.ts (1)
clix(650-652)
apps/start/src/components/overview/overview-widget-table.tsx (1)
apps/start/src/hooks/use-numer-formatter.ts (1)
useNumber(73-108)
packages/sdks/web/src/index.ts (1)
packages/sdks/sdk/src/index.ts (1)
OpenPanel(73-227)
🪛 LanguageTool
apps/public/content/docs/get-started/revenue-tracking.mdx
[grammar] ~330-~330: Ensure spelling is correct
Context: ... work if your OpenPanel instance didn't recieve a client secret (for securuty reasons)....
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~330-~330: Ensure spelling is correct
Context: ...nce didn't recieve a client secret (for securuty reasons). You can enable frontend reven...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-and-push-api
- GitHub Check: build-and-push-dashboard
- GitHub Check: build-and-push-worker
🔇 Additional comments (11)
apps/start/src/components/events/table/item.tsx (1)
57-57: LGTM! UI spacing refinements are well-executed.The spacing adjustments tighten the layout consistently:
- Reduced gaps improve visual density
- Typography adjustments maintain readability
- Padding tweaks work well for responsive design
Also applies to: 66-66, 80-80, 95-95
apps/start/src/components/widget-table.tsx (1)
93-170: Fix minWidth columns stuck hidden
getMinWidthClass()unconditionally returns the Tailwindhiddenclass, which setsdisplay: none. Your container-query CSS only toggles padding/visibility, so the display: none never gets reverted. Any column usingresponsive.minWidthwill therefore stay invisible at every breakpoint. Remove thehiddenclass (or explicitly restoredisplay) so the container query rules can reveal the column once the container is wide enough.function getMinWidthClass(minWidth: number): string { - return 'hidden'; + return ''; }apps/public/package.json (1)
6-6: LGTM!Explicitly specifying port 3001 for the dev server improves predictability and prevents port conflicts.
apps/api/src/controllers/profile.controller.ts (1)
5-5: LGTM!Good cleanup removing the unused import.
packages/sdks/web/src/index.ts (2)
23-26: LGTM!The
PendingRevenuetype is well-defined and appropriately scoped for batching revenue events.
206-229: LGTM! The revenue batching design is well-structured.The three methods (
flushRevenue,clearRevenue,pendingRevenue) provide a clean API for managing revenue events across sessions. The use of sessionStorage for persistence is appropriate for this use case.However, ensure the sessionStorage operations in
clearRevenueandpendingRevenuealso have appropriate error handling, similar to the constructor.apps/api/src/controllers/track.controller.ts (2)
110-113: LGTM!Extracting the override device ID from
properties.__deviceIdallows clients to specify their own device ID when needed, which is useful for cross-session tracking.
124-132: LGTM!The fallback logic correctly prefers the override device ID when provided, maintaining backward compatibility with the UA-based generation.
apps/api/src/routes/track.router.ts (2)
1-1: LGTM!The import is correctly added for the new endpoint.
33-47: LGTM!The new GET
/device-idroute is well-structured with:
- Appropriate HTTP method (GET for retrieval)
- Proper schema definition for the response
- Consistent use of existing hooks (clientHook, isBotHook)
packages/db/src/buffers/event-buffer.ts (1)
360-360: LGTM!The reduction from 6 to 5 KEYS aligns with the removal of last-event persistence logic (KEYS[6] for lastEventKey). This is consistent with the architectural change to source last-event data from session-level data instead.
| @@ -0,0 +1 @@ | |||
| ALTER TABLE events ADD COLUMN IF NOT EXISTS `revenue` UInt64 CODEC(Delta(4), LZ4); No newline at end of file | |||
There was a problem hiding this comment.
🧩 Analysis chain
Verify that integer-only revenue representation is intentional.
The revenue column uses UInt64, which cannot represent decimal values. This means revenue must be stored as an integer (e.g., cents or smallest currency unit). Ensure that:
- The SDK and client code convert revenue to the appropriate integer representation (e.g., dollars × 100 for cents)
- The display/query logic converts back to the decimal representation
- Documentation clearly indicates the expected unit
Run the following script to check how revenue values are being set in the codebase:
🏁 Script executed:
#!/bin/bash
# Search for revenue property assignments to verify integer handling
rg -n -C3 '__revenue|revenue.*=' --type=ts --type=jsLength of output: 6444
I can see from the output that revenue is being handled with a /100 conversion at display time (line 273 in overview-metrics.tsx), which suggests integer cents representation. Let me verify the type definitions and validation to ensure this is intentional and enforced.
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Check type definitions for revenue in TypeScript interfaces
rg -n 'revenue\s*:' --type=ts -C2 | head -60
echo "---"
# Look for revenue validation or parsing logic
rg -n 'revenue.*validat|revenue.*parse|revenue.*Decimal' --type=ts --type=js -C2 | head -40Length of output: 3869
I need to check the SDK function signature and documentation to see if revenue format is documented or enforced there.
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Find the revenue() method signature in SDK
ast-grep --pattern 'revenue($_) {
$$$
}'
echo "---"
# Look for any documentation comments around revenue
rg -n -B5 'revenue.*amount|amount.*revenue' --type=ts --type=js | grep -A5 -B5 'revenue' | head -50
echo "---"
# Check for README or documentation
fd -e md | head -10 | xargs -I {} grep -l 'revenue' {} 2>/dev/null || echo "No markdown files with revenue found"Length of output: 839
Let me get the complete method signature with any JSDoc comments:
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Get complete revenue() method including JSDoc
rg -n -B10 'revenue\s*\(' packages/sdks/sdk/src/index.ts | head -30
echo "---"
# Check if there are TypeScript branded types or Int types
rg -n 'type.*Revenue|interface.*Revenue' --type=ts -C3
echo "---"
# Check the full incoming event transformation
rg -n -B2 -A2 'body.name === .revenue.' apps/worker/src/jobs/events.incoming-event.tsLength of output: 707
Add validation and documentation for revenue cents representation.
The integer-only UInt64 representation is confirmed intentional (evidenced by the display-side division by 100 and storage schema). However, verification found three gaps:
- Missing input validation: SDK method
revenue(amount: number)and worker conversion accept floats with no validation. Passing19.99silently truncates to19instead of1999cents. - No JSDoc documentation: The SDK revenue method lacks documentation explaining that
amountmust be an integer representing cents. - No type safety: TypeScript's
numbertype permits decimals. Consider stricter typing or runtime validation.
Add JSDoc to packages/sdks/sdk/src/index.ts (revenue method) documenting the cents requirement, and implement validation in both SDK and apps/worker/src/jobs/events.incoming-event.ts to enforce integer values before storage.
lindesvard
force-pushed
the
main
branch
3 times, most recently
from
b18e60a to
8fbe944
Compare
lindesvard
force-pushed
the
feature/revenue-2
branch
from
15e476c to
04d5b0e
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/db/src/services/overview.service.ts (1)
186-227: Add null-safety to revenue field from LEFT JOIN.On line 208,
ds.total_revenuecomes from a LEFT JOIN withdaily_stats. For dates created by thefill()clause that don't have matching session data, this will beNULLin the result set. Consider usingcoalesce(ds.total_revenue, 0)to ensure consistent numeric values.Note: This same pattern exists for
ds.bounce_rateon line 201 (existing code), so this is consistent with current behavior. However, both fields would benefit from null-safety.Apply this diff to add null-safety:
- 'ds.total_revenue AS total_revenue', + 'coalesce(ds.total_revenue, 0) AS total_revenue',For consistency, consider also updating bounce_rate:
- 'ds.bounce_rate as bounce_rate', + 'coalesce(ds.bounce_rate, 0) as bounce_rate',
♻️ Duplicate comments (6)
apps/public/content/docs/get-started/revenue-tracking.mdx (1)
330-330: Fix spelling inrevenuemethod description.Line 330 still has the original typos:
recieve→receivesecuruty→securityApply:
-The revenue method will create a revenue event. It's important to know that this method will not work if your OpenPanel instance didn't recieve a client secret (for securuty reasons). You can enable frontend revenue tracking within your project settings. +The revenue method will create a revenue event. It's important to know that this method will not work if your OpenPanel instance didn't receive a client secret (for security reasons). You can enable frontend revenue tracking within your project settings.packages/sdks/web/src/index.ts (1)
40-47: GuardsessionStorageread and JSON.parse to avoid crashing the SDK.Accessing
sessionStorage.getItemandJSON.parseunguarded can throw (private mode, storage disabled/quota, corrupted/legacy values), which would break initialization on the client.Wrap this logic in a try/catch and ensure the value is a non-empty, non-
"null"string before parsing; otherwise fall back to an empty array:- const pending = sessionStorage.getItem('openpanel-pending-revenues'); - if (pending) { - this.pendingRevenues = JSON.parse(pending); - } + try { + if (typeof window !== 'undefined' && window.sessionStorage) { + const pending = window.sessionStorage.getItem( + 'openpanel-pending-revenues', + ); + if (pending && pending !== 'null') { + const parsed = JSON.parse(pending); + this.pendingRevenues = Array.isArray(parsed) ? parsed : []; + } + } + } catch { + this.pendingRevenues = []; + }Do something similar for writes below so storage errors don’t throw.
apps/start/src/components/overview/overview-metrics.tsx (2)
111-112: Remove leftover debug logging.
console.log('data', data);will spam the console on every render and may leak analytics data. It should be removed before shipping.- console.log('data', data);
293-303: Remove debug logging fordotIndex.The
console.loghere will fire on every hover/move, adding noise and potentially impacting performance:- console.log('data', dotIndex);Safe to delete.
packages/db/code-migrations/6-add-revenue-column.sql (1)
1-1: Idempotent revenue column migration looks good
ADD COLUMN IF NOT EXISTSmakes this safe on repeated runs, and the codecs are consistent with typical event columns. The earlier review comment about usingUInt64for revenue (cents representation, validation, and documentation in SDK/worker code) still applies; nothing new is required in this SQL file itself.apps/api/src/controllers/track.controller.ts (1)
380-440: Fix the incorrect error log message.As flagged in the previous review, line 433 contains a copy-pasted error message referencing the wrong endpoint:
request.log.error('Error getting session end GET /profile', error);This should reference
/device-idinstead of/profile.Apply this fix:
- request.log.error('Error getting session end GET /profile', error); + request.log.error('Error getting session end GET /device-id', error);
🧹 Nitpick comments (11)
apps/start/src/components/overview/overview-widget-table.tsx (2)
11-46: Consider clampingpercentageand marking the SVG as decorative for a11y.
RevenuePieChartlooks correct and lightweight. To make it more robust and accessible, consider:
- Clamping
percentageinto[0, 1]before computingoffsetto avoid odd visuals if an out‑of‑range value ever slips through.- Adding
aria-hidden="true"and/orrole="presentation"on the<svg>if this is purely decorative, so it doesn’t add noise for screen readers.
151-158: Revenue typing/units andhasRevenueguard – verify semantics, especially for zero/negative values.
revenueis treated as a requirednumberon pages and optional on generic rows, and both paths format values vianumber.currency(revenue / 100...). This assumes upstream data is always in the same minor‑unit (e.g. cents) scale and uses0(notnull/undefined) when there is no revenue. Please confirm that the API / data layer enforces this to avoid subtle misformatting or NaNs.- The guards
hasRevenue = data.some((item) => item.revenue > 0)/(item.revenue ?? 0) > 0plustotalRevenue > 0 ? revenue / totalRevenue : 0mean:
- The Revenue column will not render if all values are
<= 0.- Pie percentages will be 0 when
totalRevenue <= 0, even if absolute (e.g. negative refund) values exist.
If you plan to show refunds or negative net revenue, you may want a different condition (e.g.!== 0) and a share calculation that handles negative totals more explicitly.Also applies to: 164-165, 374-376, 403-429
packages/trpc/src/trpc.ts (1)
194-213: Consider disabling cache writes in non-production environments.The cache is now only read in production (line 194), but cache writes still occur in all environments (lines 206-212). This means non-production environments compute fresh data but still write to Redis unnecessarily.
Consider wrapping the cache write operation with the same environment check:
const result = await next(); // @ts-expect-error - if (result.data) { + if (result.data && process.env.NODE_ENV === 'production') { getRedisCache().setJson( key, ttl, // @ts-expect-error result.data, ); } return result;apps/start/src/hooks/use-numer-formatter.ts (1)
34-71: LGTM with a minor consideration for the currency symbol fallback.The
formatCurrencyfunction now supports both regular and compact notation formatting. The implementation is backwards-compatible since theoptionsparameter is optional.Consider making the fallback currency symbol more robust. Line 62 falls back to
'$'when the symbol cannot be extracted, which may not be appropriate for non-USD currencies:const parts = currencyFormatter.formatToParts(0); const symbol = - parts.find((part) => part.type === 'currency')?.value || '$'; + parts.find((part) => part.type === 'currency')?.value || currency; return `${formatted} ${symbol}`;This way, if the symbol extraction fails, it will fall back to the currency code (e.g., "EUR") rather than always using "$".
apps/public/content/docs/get-started/revenue-tracking.mdx (2)
323-323: Minor Markdown formatting nit in “Cons” bullet.The bold text here includes a trailing space before the closing
**:- **You can't dive deeper into where this revenue came from. **For instance, ...Consider removing the extra space so only the sentence is bold:
- **You can't dive deeper into where this revenue came from.** For instance, ...
360-364: AlignfetchDeviceIddocs with actual behavior and fix grammar.Two small issues here:
- Heading should read “Fetch your current user’s device id”.
- In the SDK snippet,
fetchDeviceIdcurrently returnsPromise<string | null>in@openpanel/sdk, so the docs should either showPromise<string | null>or clearly mention that it can returnnulland how to handle that.Example:
-### Fetch your current users device id +### Fetch your current user's device id -op.fetchDeviceId(): Promise<string> +op.fetchDeviceId(): Promise<string | null>And optionally add a note that
nullmeans the device ID could not be determined.packages/sdks/web/src/index.ts (2)
23-32: Consider typingPendingRevenue.propertiesto matchrevenueproperties.Right now
PendingRevenue.propertiesisRecord<string, unknown>, whileOpenPanelBase.revenueexpectsTrackProperties & { deviceId?: string }. Aligning these types would improve safety and editor support:-type PendingRevenue = { - amount: number; - properties?: Record<string, unknown>; -}; +type PendingRevenue = { + amount: number; + properties?: TrackProperties & { deviceId?: string }; +};This keeps pending revenue calls type-compatible with the base SDK.
206-212: Handle partial failures influshRevenuemore defensively.Using
Promise.allmeans if any singlerevenuecall rejects,flushRevenuerejects and you keep all pending revenues, potentially causing double-counts on retry.Consider:
- Logging or tracking which entries failed.
- Clearing only successfully-sent entries, or
- At least wrapping each call to avoid a single failure aborting the whole batch.
For example (simplified):
- const promises = this.pendingRevenues.map((pending) => - super.revenue(pending.amount, pending.properties), - ); - await Promise.all(promises); - this.clearRevenue(); + const results = await Promise.allSettled( + this.pendingRevenues.map((pending) => + super.revenue(pending.amount, pending.properties), + ), + ); + if (results.every((r) => r.status === 'fulfilled')) { + this.clearRevenue(); + } + // Optionally: keep failed entries only, or log failures for observability.Not strictly required, but it would make revenue tracking more robust to transient failures.
apps/start/src/components/overview/overview-metrics.tsx (1)
334-475: Revenue Y-axis domain and bar rendering are mostly fine; consider a small guard.The right Y-axis domain is computed as:
domain={[ 0, data.reduce( (max, item) => Math.max(max, item.total_revenue ?? 0), 0, ) * 2, ]}If all
total_revenuevalues are 0, the domain becomes[0, 0], which some Recharts versions treat as invalid. Consider falling back to'auto'or a minimal positive max in that case:const maxRevenue = data.reduce( (max, item) => Math.max(max, item.total_revenue ?? 0), 0, ); <YAxis {...revenueYAxisProps} yAxisId="right" orientation="right" - domain={[0, maxRevenue * 2]} + domain={maxRevenue > 0 ? [0, maxRevenue * 2] : [0, 'auto']} width={25} />The revenue
Bar+Cellsetup and the mainArea(with glow filter and dashed last segment logic) otherwise look solid.packages/db/src/session-consistency.ts (1)
212-233: Session consistency retries only help null-returning reads and comment is outdatedThis block now:
- Only has an effect for operations that return
nullon a miss (e.g.,findUnique/findFirst);findMany/aggregate/countwill never hit the retry path because their results are nevernull.- No longer forces reads to primary via
__internalParams.transaction, even though the surrounding comment still says it does.Functionally that may be fine if your main concern is read-after-write for unique lookups, but it would be good to:
- Update the comment to describe the current “retry on null result” behavior instead of “force to primary”.
- Optionally reuse the top-level retry constants instead of introducing local
MAX_RETRIES/INITIAL_RETRY_DELAY_MSso tuning stays in one place.apps/start/src/components/projects/project-chart.tsx (1)
136-142: Revenue Y-axis domain uses 2x multiplier.Line 139 sets the revenue domain to
[0, maxRevenue * 2], which caps bars at 50% height. This is likely intentional to prevent revenue bars from dominating the line chart, but consider whether a smaller multiplier (e.g., 1.5x) might improve visibility for smaller revenue values.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (42)
apps/api/src/controllers/track.controller.ts(4 hunks)apps/api/src/routes/track.router.ts(2 hunks)apps/api/src/utils/auth.ts(1 hunks)apps/public/components/flow-step.tsx(1 hunks)apps/public/content/docs/get-started/revenue-tracking.mdx(1 hunks)apps/public/package.json(1 hunks)apps/start/src/components/chart-ssr.tsx(0 hunks)apps/start/src/components/charts/chart-tooltip.tsx(2 hunks)apps/start/src/components/charts/common-bar.tsx(1 hunks)apps/start/src/components/events/event-icon.tsx(1 hunks)apps/start/src/components/events/table/item.tsx(4 hunks)apps/start/src/components/overview/overview-live-histogram.tsx(1 hunks)apps/start/src/components/overview/overview-metric-card.tsx(2 hunks)apps/start/src/components/overview/overview-metrics.tsx(12 hunks)apps/start/src/components/overview/overview-widget-table.tsx(8 hunks)apps/start/src/components/projects/project-card.tsx(7 hunks)apps/start/src/components/projects/project-chart.tsx(1 hunks)apps/start/src/components/realtime/realtime-live-histogram.tsx(1 hunks)apps/start/src/components/realtime/realtime-reloader.tsx(1 hunks)apps/start/src/components/settings/edit-project-details.tsx(3 hunks)apps/start/src/components/widget-table.tsx(4 hunks)apps/start/src/hooks/use-numer-formatter.ts(1 hunks)apps/worker/src/jobs/events.create-session-end.ts(5 hunks)apps/worker/src/jobs/events.incoming-event.ts(5 hunks)apps/worker/src/jobs/events.incoming-events.test.ts(7 hunks)packages/db/code-migrations/6-add-revenue-column.sql(1 hunks)packages/db/code-migrations/6-add-revenue-column.ts(1 hunks)packages/db/prisma/migrations/20251118100123_add_revenue_tracking_setting_on_project/migration.sql(1 hunks)packages/db/prisma/schema.prisma(1 hunks)packages/db/src/buffers/session-buffer.ts(5 hunks)packages/db/src/services/chart.service.ts(1 hunks)packages/db/src/services/event.service.ts(5 hunks)packages/db/src/services/overview.service.ts(16 hunks)packages/db/src/session-consistency.ts(1 hunks)packages/sdks/sdk/src/index.ts(1 hunks)packages/sdks/web/src/index.ts(3 hunks)packages/sdks/web/src/types.d.ts(1 hunks)packages/trpc/src/routers/chart.ts(2 hunks)packages/trpc/src/routers/overview.ts(4 hunks)packages/trpc/src/routers/project.ts(2 hunks)packages/trpc/src/trpc.ts(1 hunks)packages/validation/src/index.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/start/src/components/chart-ssr.tsx
✅ Files skipped from review due to trivial changes (1)
- apps/start/src/components/overview/overview-live-histogram.tsx
🚧 Files skipped from review as they are similar to previous changes (10)
- apps/start/src/components/overview/overview-metric-card.tsx
- apps/api/src/routes/track.router.ts
- apps/public/package.json
- packages/db/src/buffers/session-buffer.ts
- apps/worker/src/jobs/events.create-session-end.ts
- packages/db/code-migrations/6-add-revenue-column.ts
- packages/db/src/services/event.service.ts
- apps/start/src/components/realtime/realtime-live-histogram.tsx
- packages/sdks/sdk/src/index.ts
- apps/public/components/flow-step.tsx
🧰 Additional context used
📓 Path-based instructions (1)
apps/start/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/start/.cursorrules)
apps/start/**/*.{ts,tsx}: Instrument server functions created with createServerFn by wrapping their implementation in Sentry.startSpan and importing Sentry from '@sentry/tanstackstart-react'
When using Sentry in the TanStack Start React app, import it as: import * as Sentry from '@sentry/tanstackstart-react'
Files:
apps/start/src/components/projects/project-chart.tsxapps/start/src/components/widget-table.tsxapps/start/src/components/charts/chart-tooltip.tsxapps/start/src/components/events/table/item.tsxapps/start/src/hooks/use-numer-formatter.tsapps/start/src/components/overview/overview-widget-table.tsxapps/start/src/components/events/event-icon.tsxapps/start/src/components/overview/overview-metrics.tsxapps/start/src/components/charts/common-bar.tsxapps/start/src/components/realtime/realtime-reloader.tsxapps/start/src/components/settings/edit-project-details.tsxapps/start/src/components/projects/project-card.tsx
🧠 Learnings (2)
📚 Learning: 2025-09-26T18:30:17.476Z
Learnt from: CR
Repo: Openpanel-dev/openpanel PR: 0
File: .cursorrules:0-0
Timestamp: 2025-09-26T18:30:17.476Z
Learning: When writing ClickHouse queries, use the helper functions in ./packages/db/src/clickhouse/query-functions.ts rather than crafting raw queries
Applied to files:
apps/worker/src/jobs/events.incoming-events.test.ts
📚 Learning: 2025-09-26T18:30:17.476Z
Learnt from: CR
Repo: Openpanel-dev/openpanel PR: 0
File: .cursorrules:0-0
Timestamp: 2025-09-26T18:30:17.476Z
Learning: When writing ClickHouse queries, always construct them using the custom query builder in ./packages/db/src/clickhouse/query-builder.ts
Applied to files:
apps/worker/src/jobs/events.incoming-events.test.ts
🧬 Code graph analysis (12)
apps/start/src/components/projects/project-chart.tsx (4)
apps/start/src/components/charts/chart-tooltip.tsx (3)
createChartTooltip(41-95)ChartTooltipHeader(16-20)ChartTooltipItem(22-39)apps/start/src/hooks/use-numer-formatter.ts (1)
useNumber(73-108)apps/start/src/utils/theme.ts (1)
getChartColor(25-27)apps/public/lib/utils.ts (1)
cn(4-6)
apps/worker/src/jobs/events.incoming-event.ts (2)
packages/sdks/sdk/src/index.ts (1)
revenue(186-197)packages/db/src/buffers/index.ts (1)
sessionBuffer(9-9)
apps/start/src/components/widget-table.tsx (1)
apps/public/lib/utils.ts (1)
cn(4-6)
apps/start/src/components/charts/chart-tooltip.tsx (1)
apps/public/lib/utils.ts (1)
cn(4-6)
packages/db/src/services/overview.service.ts (1)
packages/db/src/clickhouse/query-builder.ts (1)
clix(650-652)
apps/start/src/components/overview/overview-widget-table.tsx (1)
apps/start/src/hooks/use-numer-formatter.ts (1)
useNumber(73-108)
apps/api/src/controllers/track.controller.ts (3)
packages/sdks/sdk/src/index.ts (1)
fetchDeviceId(199-206)packages/db/src/services/salt.service.ts (1)
getSalts(20-49)packages/redis/redis.ts (1)
getRedisCache(66-72)
packages/trpc/src/routers/chart.ts (3)
packages/db/src/services/organization.service.ts (1)
getSettingsForProject(292-305)packages/db/src/clickhouse/client.ts (3)
chQuery(257-262)TABLE_NAMES(49-61)ch(155-189)packages/db/src/clickhouse/query-builder.ts (1)
clix(650-652)
apps/start/src/components/overview/overview-metrics.tsx (3)
packages/common/src/get-previous-metric.ts (1)
getPreviousMetric(7-39)apps/start/src/hooks/use-numer-formatter.ts (1)
useNumber(73-108)apps/start/src/utils/theme.ts (1)
getChartColor(25-27)
apps/worker/src/jobs/events.incoming-events.test.ts (3)
packages/db/src/buffers/index.ts (1)
sessionBuffer(9-9)packages/db/src/clickhouse/client.ts (1)
formatClickhouseDate(264-275)packages/db/src/services/session.service.ts (1)
IClickhouseSession(17-57)
packages/sdks/web/src/index.ts (1)
packages/sdks/sdk/src/index.ts (1)
OpenPanel(73-227)
apps/start/src/components/projects/project-card.tsx (2)
apps/start/src/components/projects/project-chart.tsx (1)
ProjectChart(87-215)apps/start/src/hooks/use-numer-formatter.ts (1)
useNumber(73-108)
🪛 LanguageTool
apps/public/content/docs/get-started/revenue-tracking.mdx
[grammar] ~330-~330: Ensure spelling is correct
Context: ... work if your OpenPanel instance didn't recieve a client secret (for securuty reasons)....
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~330-~330: Ensure spelling is correct
Context: ...nce didn't recieve a client secret (for securuty reasons). You can enable frontend reven...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-and-push-api
- GitHub Check: build-and-push-dashboard
- GitHub Check: build-and-push-worker
🔇 Additional comments (39)
apps/start/src/components/charts/common-bar.tsx (1)
80-88: LGTM! Clean addition of green bar variant.The implementation correctly follows the established pattern for color variants. The green color scheme is well-suited for revenue visualization as indicated in the PR objectives.
apps/start/src/components/overview/overview-widget-table.tsx (2)
79-88: Column base class composition and comment look consistent with the newresponsiveprop.Using
cnto apply alignment and typography per index while delegating visibility to theresponsiveprop keeps concerns nicely separated and matches the comment about removing old responsive logic.
172-273: Responsive priorities across pages and generic tables appear coherent.The chosen
priorityvalues (Path/primary column = 1, Sessions = 2, Revenue = 3, BR/Duration with higher numbers) match the comments about “always visible” vs. “hidden when space is tight” and give a sensible ordering of importance. AssumingWidgetTableinterprets lower numbers as higher priority, this aligns well with the intended behavior on constrained layouts.Also applies to: 383-444
apps/start/src/components/widget-table.tsx (1)
2-19: ColumnResponsive API and prop wiring look soundThe
ColumnPriority/ColumnResponsivetypes and the newresponsivefield oncolumnsare well-shaped and documented, and the default behavior (“always visible” whenresponsiveis omitted) is clear and consistent with the implementation.Also applies to: 27-31
apps/start/src/components/events/event-icon.tsx (1)
51-54: LGTM!The revenue icon mapping is properly configured with
DollarSignIconand green color, both of which are already available in theEventIconMapperandEventIconColorsarrays.apps/start/src/components/charts/chart-tooltip.tsx (1)
22-39: LGTM!The optional styling props are well-designed, allowing consumers to customize both the outer container and inner content wrapper. The
cnutility properly merges the default classes with custom ones.apps/start/src/components/events/table/item.tsx (1)
57-95: LGTM!The spacing adjustments create a more compact and consistent event list item layout. The removal of the "Event:" label in the non-screen_view branch simplifies the display while maintaining clarity.
packages/db/src/services/chart.service.ts (1)
176-214: LGTM!The revenue-specific handling is consistently implemented across all property aggregation segments (sum, average, max, min). Using the
revenuecolumn directly withouttoFloat64conversion is appropriate since it's a numeric column in the database schema, and therevenue > 0filter ensures only relevant events are included.apps/start/src/components/realtime/realtime-reloader.tsx (1)
17-28: LGTM!The query filter updates expand the realtime data refresh mechanism to include
referralsandpathswhile updating the existingliveDataandactiveSessionsfilters. The refetch pattern remains consistent with the visibility-based triggering logic.packages/sdks/web/src/types.d.ts (1)
11-15: All revenue methods are properly implemented.Verification confirms that
revenue,flushRevenue,clearRevenue, andpendingRevenueare all implemented in the web SDK (packages/sdks/web/src/index.ts). The type definitions at lines 12-15 inpackages/sdks/web/src/types.d.tscorrectly match the implementations, with no gaps between declared types and actual methods.packages/trpc/src/routers/chart.ts (2)
86-101: Revenue aggregation in metrics looks consistent.
metricsPromisenow selects:'sum(revenue * sign) as revenue',from
TABLE_NAMES.sessionsover a 6‑month window. This matches the chart query’s revenue aggregation and extends the metrics shape with arevenuefield, which is coherent with the rest of the PR.No changes needed here.
217-240: Includingrevenuein the properties list is appropriate.Adding
'revenue'to the static properties array ensures revenue can be used as a first‑class property in filters and property pickers:properties.push( 'revenue', 'has_profile', 'path', ... );This aligns with the new revenue column on events/sessions and with the rest of the revenue-aware charting changes.
apps/start/src/components/overview/overview-metrics.tsx (3)
79-85: Revenue metric wiring looks consistent with the rest of the UI.The new
Revenueentry inTITLESuses keytotal_revenueand unit'currency', which is handled specially in the tooltip and chart rendering. This matches the backend’stotal_revenue/prev_total_revenuefields and the currency formatting viauseNumber.No changes needed here.
183-252: Tooltip currency and revenue section behavior looks good.
- Using
metric.unit === 'currency'to decide betweennumber.currency((data[metric.key] ?? 0) / 100)andformatWithUnitis correct and keeps non-currency metrics unchanged.- The additional revenue block conditioned on
revenue > 0cleanly surfacestotal_revenue/prev_total_revenuewith the same currency formatter andPreviousDiffIndicatorPure.No changes needed here.
272-275: Using a separate Y-axis for revenue ticks is appropriate.The
revenueYAxisPropswith atickFormatterdividing by 100 to convert cents to base currency units is aligned with the tooltip and card formatting:const revenueYAxisProps = useYAxisProps({ tickFormatter: (value) => number.short(value / 100), });This makes the right-side revenue axis readable while still working with cent-based values in the data.
packages/db/prisma/migrations/20251118100123_add_revenue_tracking_setting_on_project/migration.sql (1)
1-2: Migration forallowUnsafeRevenueTrackinglooks correct.Adding a
BOOLEAN NOT NULL DEFAULT falsecolumn onpublic.projectsis safe for existing data and fits the intended feature flag for unsafe revenue tracking.No changes needed.
packages/validation/src/index.ts (1)
375-383: allowUnsafeRevenueTracking wiring into zProject looks correctAdding
allowUnsafeRevenueTracking: z.boolean().default(false)keeps projects safe by default and aligns with how the TRPC project router updates and creates projects. This cleanly exposes the flag for edits without changing existing behavior.packages/trpc/src/routers/project.ts (1)
68-88: allowUnsafeRevenueTracking handling in update/create is consistent and safeUpdate passes
input.allowUnsafeRevenueTrackingthrough (soundefinedleaves the DB value unchanged), and create explicitly initializes it tofalse, matching the safe default in validation/DB. This cleanly supports toggling the flag later via edits while keeping onboarding projects locked down by default.Also applies to: 120-129
apps/start/src/components/settings/edit-project-details.tsx (2)
29-29: LGTM! Field integration is consistent.The
allowUnsafeRevenueTrackingfield is properly added to both the validator and form defaults, following the same pattern as other boolean fields likecrossDomain.Also applies to: 43-43
178-198: LGTM! Clear security signaling with "unsafe" terminology.The new revenue tracking section follows the established pattern and clearly communicates the security implications of enabling client-side revenue tracking.
apps/api/src/controllers/track.controller.ts (2)
119-122: LGTM! Consistent property extraction pattern.The
overrideDeviceIdextraction follows the same pattern as other special properties like__ipand__timestamp, allowing the SDK to specify a deviceId when needed.
133-141: LGTM! Correct precedence for deviceId resolution.The logic appropriately prioritizes
overrideDeviceIdwhen provided, which enables the SDK to maintain consistent device identification for revenue events.packages/trpc/src/routers/overview.ts (2)
100-163: LGTM! Session counting logic updated consistently.The shift from
count(*)onsession_startevents touniq(session_id)across all events is a semantic change that's consistently applied throughout the live data queries. This aligns with the broader session-centric refactoring in this PR.
233-233: LGTM! Revenue fields added consistently.The
prev_total_revenuefields are added following the same pattern as other previous-period metrics, maintaining consistency in the API response structure.Also applies to: 246-246
apps/start/src/components/projects/project-chart.tsx (1)
27-85: LGTM! Tooltip implementation is clean and user-friendly.The tooltip conditionally displays revenue only when present and formats all values appropriately using locale-aware formatters.
packages/db/prisma/schema.prisma (1)
184-184: LGTM! Secure default for revenue tracking flag.The
allowUnsafeRevenueTrackingfield with@default(false)ensures new projects don't allow client-side revenue tracking unless explicitly enabled.apps/worker/src/jobs/events.incoming-event.ts (4)
44-53: LGTM! Defensive revenue parsing.The
parseRevenuefunction safely handles multiple input types and returnsundefinedfor invalid values, preventing type errors downstream.
129-132: LGTM! Revenue only populated for revenue events.The conditional logic ensures the
revenuefield is only set when the event is explicitly a revenue event and the__revenueproperty is present.
137-164: LGTM! Session-centric event enrichment for server-side events.The migration from
eventBuffertosessionBuffer.getExistingSessionprovides richer context for server-side and past-timestamp events. The fallback pattern ensures events are created even when session data is unavailable.
178-191: LGTM! Correct use of exit paths for ending events.Switching to
exit_pathandexit_originfrom the session buffer is semantically correct for events that may occur after a user has navigated away, ensuring accurate path attribution.apps/start/src/components/projects/project-card.tsx (2)
14-14: LGTM! Clean extraction of chart component.The refactor to use an external
ProjectChartcomponent improves reusability while maintaining clear naming with theProjectChartOuterwrapper.Also applies to: 63-63, 90-90
142-153: LGTM! Improved number formatting with conditional revenue display.The switch to the
useNumberhook provides a cleaner API, and the revenue metric is appropriately displayed only when data is present. Currency formatting withshort: trueis well-suited for compact card layouts.apps/worker/src/jobs/events.incoming-events.test.ts (2)
1-29: LGTM! Test mocks updated to reflect sessionBuffer migration.The mock structure correctly reflects the production code's migration from
eventBuffer.getLastScreenViewtosessionBuffer.getExistingSession, maintaining comprehensive test coverage.
115-115: LGTM! Comprehensive test coverage for revenue and session data.The test expectations correctly include
revenue: undefinedfor non-revenue events, and theIClickhouseSessionmock is detailed and realistic, ensuring thorough test coverage of the sessionBuffer integration.Also applies to: 220-220, 289-366, 411-411
packages/db/src/services/overview.service.ts (5)
100-118: LGTM! Return type updated correctly.The addition of
total_revenueto both metrics and series structures is consistent with other numeric fields.
123-139: LGTM! Revenue aggregation follows ClickHouse patterns.The
sum(revenue * sign)aggregation is correct for ClickHouse's versioned table pattern with thesigncolumn.
171-184: LGTM! Revenue CTEs follow existing patterns.The
overall_total_revenueanddaily_statsCTEs correctly separate rollup aggregates from per-date series, consistent with the existingoverall_bounce_ratepattern.
282-349: LGTM! Non-page-filter path handles revenue correctly.This path aggregates revenue directly with
sum(revenue * sign)and safely handles potential undefined values with the?? 0fallback on line 342.
382-633: LGTM! Revenue tracking added consistently across top queries.The revenue field is properly integrated into
getTopPages,getTopEntryExit, andgetTopGeneric:
- Consistent use of
sum(revenue * sign)aggregation pattern- Good null-safety with
coalesce(b.revenue, 0)on line 449 for the LEFT JOIN ingetTopPages- Return types correctly updated to include revenue
| /** | ||
| * Generates container query class based on priority. | ||
| * Lower priority numbers = hidden at smaller widths. | ||
| * Priority 1 = always visible (highest priority) | ||
| * Priority 10 = hidden first (lowest priority) | ||
| */ | ||
| function getResponsiveClass(priority: ColumnPriority): string { | ||
| // Priority 1 = always visible (no hiding) | ||
| if (priority === 1) { | ||
| return ''; | ||
| } | ||
|
|
||
| // Columns will be hidden via CSS container queries | ||
| // Return empty string - hiding is handled by CSS | ||
| return ''; | ||
| } | ||
|
|
||
| function getResponsiveStyle( | ||
| priority: ColumnPriority, | ||
| ): React.CSSProperties | undefined { | ||
| if (priority === 1) { | ||
| return undefined; | ||
| } | ||
|
|
||
| const minWidth = (priority - 1) * 100 + 100; | ||
| return { | ||
| // Use CSS custom property for container query | ||
| // Will be handled by inline style with container query | ||
| } as React.CSSProperties; | ||
| } | ||
|
|
||
| /** | ||
| * Generates container query class based on custom min-width. | ||
| */ | ||
| function getMinWidthClass(minWidth: number): string { | ||
| return 'hidden'; | ||
| } | ||
|
|
There was a problem hiding this comment.
minWidth-based columns will stay permanently hidden; also some helper dead code
Two issues in the responsive helpers:
- Functional bug for
minWidthcolumns
getMinWidthClass currently returns 'hidden':
function getMinWidthClass(minWidth: number): string {
return 'hidden';
}Since Tailwind’s hidden maps to display: none, any column using responsive.minWidth will remain invisible at all container widths: the container-query rules only toggle min-width, max-width, padding, overflow, and visibility, but never override display. So those cells won’t reappear even when the container becomes wide enough.
Given you already generate base “hidden by default, visible at @container(min-width …)” rules in containerQueryStyles, the helper should not apply hidden at all.
A minimal fix:
-function getMinWidthClass(minWidth: number): string {
- return 'hidden';
-}
+function getMinWidthClass(minWidth: number): string {
+ // Visibility is controlled via container queries; avoid adding `hidden`
+ // (display: none), which would prevent the column from ever showing.
+ return '';
+}- Redundant / unused helpers
getResponsiveClassalways returns'', so it doesn’t currently contribute anything beyond whatcontainerQueryStylesdoes. That’s fine functionally but a bit misleading given the JSDoc; consider either simplifying the comment or removing the helper once behavior stabilizes.getResponsiveStyleis never used; it’s effectively dead code and could be removed to keep the file lean.
🤖 Prompt for AI Agents
In apps/start/src/components/widget-table.tsx around lines 59-96,
getMinWidthClass currently returns the string 'hidden', which causes
minWidth-based columns to be permanently display:none; change getMinWidthClass
to return an empty string (or otherwise not emit the hidden class) so that the
container-query CSS rules can control visibility, and remove or consolidate the
dead/unused helpers (getResponsiveStyle and/or getResponsiveClass) or update
their comments to reflect current behavior to keep the file lean.
| const containerId = React.useMemo( | ||
| () => `widget-table-${Math.random().toString(36).substring(7)}`, | ||
| [], | ||
| ); | ||
|
|
||
| // Generate CSS for container queries | ||
| const containerQueryStyles = React.useMemo(() => { | ||
| const styles: string[] = []; | ||
|
|
||
| columns.forEach((column) => { | ||
| if ( | ||
| column.responsive?.priority !== undefined && | ||
| column.responsive.priority > 1 | ||
| ) { | ||
| // Breakpoints - Priority 2 = 150px, Priority 3 = 250px, etc. | ||
| // Less aggressive: columns show at smaller container widths | ||
| const minWidth = (column.responsive.priority - 1) * 100 + 50; | ||
| // Hide by default by collapsing width and hiding content | ||
| // Keep in grid flow but take up minimal space | ||
| styles.push( | ||
| `.${containerId} .cell[data-priority="${column.responsive.priority}"] { min-width: 0; max-width: 0; padding-left: 0; padding-right: 0; overflow: hidden; visibility: hidden; }`, | ||
| `@container (min-width: ${minWidth}px) { .${containerId} .cell[data-priority="${column.responsive.priority}"] { min-width: revert; max-width: revert; padding-left: revert; padding-right: 0.5rem; overflow: revert; visibility: visible !important; } }`, | ||
| ); | ||
| } else if (column.responsive?.minWidth !== undefined) { | ||
| styles.push( | ||
| `.${containerId} .cell[data-min-width="${column.responsive.minWidth}"] { min-width: 0; max-width: 0; padding-left: 0; padding-right: 0; overflow: hidden; visibility: hidden; }`, | ||
| `@container (min-width: ${column.responsive.minWidth}px) { .${containerId} .cell[data-min-width="${column.responsive.minWidth}"] { min-width: revert; max-width: revert; padding-left: revert; padding-right: 0.5rem; overflow: revert; visibility: visible !important; } }`, | ||
| ); | ||
| } | ||
| }); | ||
|
|
||
| // Ensure last visible cell always has padding-right | ||
| styles.push( | ||
| `.${containerId} .cell:last-child { padding-right: 1rem !important; }`, | ||
| ); | ||
|
|
||
| return styles.length > 0 ? <style>{styles.join('\n')}</style> : null; | ||
| }, [columns, containerId]); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Random containerId risks SSR hydration mismatches; prefer a deterministic id
containerId is derived from Math.random():
const containerId = React.useMemo(
() => `widget-table-${Math.random().toString(36).substring(7)}`,
[],
);In an SSR + hydration setup (which TanStack Start uses), this means the class used in the rendered HTML differs between server and client, causing hydration warnings and forcing React to patch the DOM. It also makes CSS less predictable for streaming/rehydration.
You can keep the per-instance uniqueness while making it deterministic by basing it on React.useId() and sanitizing it for CSS-class usage:
- const containerId = React.useMemo(
- () => `widget-table-${Math.random().toString(36).substring(7)}`,
- [],
- );
+ const containerId = `widget-table-${React.useId().replace(/[:]/g, '')}`;containerQueryStyles will continue to work unchanged, since it already uses .${containerId} .cell[...] and the wrapper div applies containerId as a class.
Also applies to: 154-159
🌐 Web query:
Confirm that the React version used in this repo supports `React.useId()` (React 18+), and that existing tooling/type definitions include container-query CSS properties such as `containerType`.
💡 Result:
I don't have the repo. Please either paste the repo URL or the relevant files (package.json / package-lock.json / yarn.lock / pnpm-lock.yaml and tsconfig / any CSS type files). I can then:
- Check the installed React version to confirm React.useId() (requires React 18+).
- Inspect installed @types/react and other tooling (postcss, stylelint, TypeScript lib.dom types) to confirm container-query CSS properties like containerType are present.
If you prefer to check locally, paste the output of:
- cat package.json (or share dependencies/devDependencies lines)
- npm ls react @types/react
- npm ls typescript
and (if TypeScript) the value of "lib" in tsconfig.json.
I'll inspect the repository to verify the React version and tooling configuration.
---
🏁 Script executed:
```shell
#!/bin/bash
# Check package.json for React version and dependencies
cat package.json | grep -A 50 '"dependencies"' | head -60
Length of output: 678
🏁 Script executed:
#!/bin/bash
# Check TypeScript version and tsconfig.json lib configuration
cat tsconfig.json | grep -A 5 '"lib"'Length of output: 45
🏁 Script executed:
#!/bin/bash
# Verify the file under review exists and check its current state around lines 113-151
head -160 apps/start/src/components/widget-table.tsx | tail -60Length of output: 2520
I need to check the workspace-level configuration. Let me look at the apps/start package.json and tsconfig.json:
---
🏁 Script executed:
```shell
#!/bin/bash
# Check apps/start package.json for React version
cat apps/start/package.json | grep -A 100 '"dependencies"'
Length of output: 3738
🏁 Script executed:
#!/bin/bash
# Check apps/start tsconfig.json
cat apps/start/tsconfig.jsonLength of output: 642
🏁 Script executed:
#!/bin/bash
# Check for pnpm-lock.yaml to verify React version
grep -A 2 "react@" pnpm-lock.yaml | head -20Length of output: 915
Replace Math.random()-based containerId with React.useId() for SSR compatibility
The current implementation uses Math.random() to generate containerId, which causes SSR hydration mismatches in TanStack Start (client-generated ID differs from server-rendered ID).
Replace with:
- const containerId = React.useMemo(
- () => `widget-table-${Math.random().toString(36).substring(7)}`,
- [],
- );
+ const containerId = `widget-table-${React.useId().replace(/[:]/g, '')}`;Verification confirms React 19.1.1 is installed (supports React.useId() since React 18+), TypeScript includes DOM types with container-query properties, and the codebase already uses container queries (containerType: 'inline-size'). This refactor is fully compatible.
Also applies to: 154-159
🤖 Prompt for AI Agents
In apps/start/src/components/widget-table.tsx around lines 113 to 151 (and also
apply the same change at 154-159), replace the Math.random()-based containerId
with React.useId() to avoid SSR hydration mismatches; specifically, remove the
React.useMemo that builds a random id and instead call React.useId() (optionally
prefixing it like `widget-table-`), then use that id in the generated CSS
strings where containerId is referenced so server and client IDs match.
| {columns.map((column, colIndex) => { | ||
| const responsiveClass = | ||
| column.responsive?.priority !== undefined | ||
| ? getResponsiveClass(column.responsive.priority) | ||
| : column.responsive?.minWidth !== undefined | ||
| ? getMinWidthClass(column.responsive.minWidth) | ||
| : ''; | ||
|
|
||
| const dataAttrs: Record<string, string> = {}; | ||
| if (column.responsive?.priority !== undefined) { | ||
| dataAttrs['data-priority'] = String(column.responsive.priority); | ||
| } | ||
| if (column.responsive?.minWidth !== undefined) { | ||
| dataAttrs['data-min-width'] = String(column.responsive.minWidth); | ||
| } | ||
|
|
||
| return ( | ||
| <div | ||
| key={column.name?.toString()} | ||
| className={cn( | ||
| 'p-2 font-medium font-sans text-sm whitespace-nowrap cell', | ||
| columns.length > 1 && column !== columns[0] | ||
| ? 'text-right' | ||
| : 'text-left', | ||
| column.className, | ||
| responsiveClass, | ||
| )} | ||
| style={{ width: column.width }} | ||
| {...dataAttrs} | ||
| > | ||
| {column.name} | ||
| </div> | ||
| ); | ||
| })} |
There was a problem hiding this comment.
Column rendering: small correctness and ergonomics tweaks (keys, alignment, width handling)
A few points in the header/body column maps that are worth tightening up:
- Unused
colIndex/ brittle alignment check
You compute colIndex but then check column !== columns[0] to decide alignment:
columns.length > 1 && column !== columns[0]
? 'text-right'
: 'text-left'For clarity (and to actually use colIndex), this is simpler and more explicit as colIndex > 0:
- columns.length > 1 && column !== columns[0]
- ? 'text-right'
- : 'text-left',
+ columns.length > 1 && colIndex > 0
+ ? 'text-right'
+ : 'text-left',Apply the same change in the body renderer.
- Keys based on
column.name?.toString()
Using column.name?.toString() as the key is fragile when name is a ReactNode (e.g. an icon or element), and can lead to duplicated or unstable keys. Since column definitions are usually static, an index-based key is acceptable and more predictable here:
- key={column.name?.toString()}
+ key={colIndex}Likewise for the body cells.
- Header
widthhandling is inconsistent with body and may be invalid forw-full
In the header you always do:
style={{ width: column.width }}but in the body you special-case 'w-full':
column.width === 'w-full' && 'w-full min-w-0',
style={column.width !== 'w-full' ? { width: column.width } : {}}If any callers pass Tailwind width classes like 'w-full', the header ends up with width: 'w-full', which is invalid CSS. To keep behavior consistent and avoid invalid styles, mirror the body logic in the header:
- className={cn(
- 'p-2 font-medium font-sans text-sm whitespace-nowrap cell',
- columns.length > 1 && colIndex > 0 ? 'text-right' : 'text-left',
- column.className,
- responsiveClass,
- )}
- style={{ width: column.width }}
+ className={cn(
+ 'p-2 font-medium font-sans text-sm whitespace-nowrap cell',
+ columns.length > 1 && colIndex > 0 ? 'text-right' : 'text-left',
+ column.className,
+ column.width === 'w-full' && 'w-full min-w-0',
+ responsiveClass,
+ )}
+ style={
+ column.width !== 'w-full' ? { width: column.width } : {}
+ }This keeps the layout model consistent between header and body and avoids invalid inline styles.
Also applies to: 215-255
🤖 Prompt for AI Agents
In apps/start/src/components/widget-table.tsx around lines 164-197 (and
similarly apply to 215-255), the header column rendering should (1) use the
provided colIndex for alignment checks instead of comparing object identity, (2)
use a stable index-based key (e.g., colIndex) instead of
column.name?.toString(), and (3) mirror the body width handling so Tailwind
width classes like 'w-full' are not emitted as invalid inline styles — i.e., if
column.width === 'w-full' add the class (e.g., 'w-full min-w-0') and don't set
style.width, otherwise set style.width to column.width; apply the same colIndex
alignment and key fixes to the body renderer as well.
| const { timezone } = await getSettingsForProject(projectId); | ||
| const chartPromise = chQuery<{ | ||
| value: number; | ||
| date: Date; | ||
| revenue: number; | ||
| }>( | ||
| `SELECT | ||
| uniqHLL12(profile_id) as value, | ||
| toStartOfDay(created_at) as date | ||
| toStartOfDay(created_at) as date, | ||
| sum(revenue * sign) as revenue | ||
| FROM ${TABLE_NAMES.sessions} | ||
| WHERE | ||
| sign = 1 AND | ||
| project_id = ${sqlstring.escape(projectId)} AND | ||
| created_at >= now() - interval '3 month' | ||
| GROUP BY date | ||
| ORDER BY date ASC | ||
| WITH FILL FROM toStartOfDay(now() - interval '1 month') | ||
| TO toStartOfDay(now()) | ||
| STEP INTERVAL 1 day | ||
| SETTINGS session_timezone = '${timezone}' | ||
| `, |
There was a problem hiding this comment.
🧩 Analysis chain
Fix ClickHouse interval syntax and escape timezone.
The raw SQL in projectCard looks like Postgres-style intervals:
created_at >= now() - interval '3 month'
...
WITH FILL FROM toStartOfDay(now() - interval '1 month')ClickHouse expects INTERVAL without quotes, e.g. now() - INTERVAL 3 MONTH. As written, this query is very likely to fail at runtime.
Also, session_timezone = '${timezone}' interpolates a DB value directly into SQL; use sqlstring.escape to avoid injection from a misconfigured timezone string.
Suggested fix:
- created_at >= now() - interval '3 month'
+ created_at >= now() - INTERVAL 3 MONTH
...
- WITH FILL FROM toStartOfDay(now() - interval '1 month')
- TO toStartOfDay(now())
+ WITH FILL FROM toStartOfDay(now() - INTERVAL 1 MONTH)
+ TO toStartOfDay(now())
...
- SETTINGS session_timezone = '${timezone}'
+ SETTINGS session_timezone = ${sqlstring.escape(timezone)}This should make the query valid for ClickHouse and safer.
I'll verify the review comment by examining the actual code and checking ClickHouse documentation.
🏁 Script executed:
#!/bin/bash
# Examine the file to see the actual code at the specified lines
if [ -f "packages/trpc/src/routers/chart.ts" ]; then
echo "=== File content (lines 60-90) ==="
sed -n '60,90p' packages/trpc/src/routers/chart.ts
else
echo "File not found at packages/trpc/src/routers/chart.ts"
find . -name "chart.ts" -type f | head -5
fiLength of output: 990
🌐 Web query:
ClickHouse INTERVAL syntax correct format
💡 Result:
Short answer — valid ClickHouse INTERVAL forms
-
Keyword form: INTERVAL
- Example: INTERVAL 4 DAY, INTERVAL 10 HOUR. [1]
-
Quoted/string form: INTERVAL '' or INTERVAL ''
- Examples: INTERVAL '1' hour, INTERVAL '1 hour', INTERVAL '1 HOUR 30 MINUTE' (composite). [1][3]
-
Programmatic constructors (equivalent): toIntervalSecond(n), toIntervalMinute(n), toIntervalHour(n), toIntervalDay(n), toIntervalWeek(n), toIntervalMonth(n), toIntervalQuarter(n), toIntervalYear(n). Use these when you need explicit Interval-typed values. Example: toIntervalMonth(1). [2]
Notes
- Different interval types cannot be combined directly without using the composite/tuple syntax; use separate INTERVAL terms added together or the quoted composite form / tuple of intervals. [1][3]
Sources: ClickHouse docs — INTERVAL/operator and date-time functions. [1][2][3]
Escape timezone parameter to prevent SQL injection.
The session_timezone setting interpolates the timezone value directly without escaping: ClickHouse supports both INTERVAL 3 MONTH (unquoted) and INTERVAL '3 month' (quoted) forms, so the current interval syntax is valid. However, the timezone should be escaped using sqlstring.escape() to mitigate potential SQL injection if the timezone value is unexpectedly malformed.
Suggested fix:
- SETTINGS session_timezone = '${timezone}'
+ SETTINGS session_timezone = ${sqlstring.escape(timezone)}🤖 Prompt for AI Agents
In packages/trpc/src/routers/chart.ts around lines 63 to 83, the
session_timezone setting interpolates the timezone string directly into the SQL
which risks SQL injection; escape the timezone before interpolation (e.g., pass
timezone through sqlstring.escape or equivalent and use the escaped value in the
SETTINGS clause) so the constructed query uses a properly quoted/escaped
timezone value.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
apps/api/src/controllers/track.controller.ts (1)
433-433: Fix incorrect error log message.The error log references
/profilebut should reference/device-idor/track/device-idas this is the actual endpoint.Based on past review comments.
Apply this diff:
- request.log.error('Error getting session end GET /track/device-id', error); + request.log.error('Error checking session existence for GET /track/device-id', error);
🧹 Nitpick comments (1)
apps/worker/src/jobs/events.create-session-end.ts (1)
45-62: Session retrieval correctly updated to match new API.The call now properly passes only
{ sessionId }instead of bothprojectIdandsessionId, resolving the previous critical signature mismatch. The transformation viatransformSessionToEventand null filtering withflatMapare correct.Minor point: The variable
lastScreenViewis misleading since it now holds a session object rather than a screen view event.Consider renaming for clarity:
- const [lastScreenView, eventsInDb] = await Promise.all([ + const [session, eventsInDb] = await Promise.all([ sessionBuffer.getExistingSession({ sessionId, }), getEvents(sql), ]); // sort last inserted first return [ - lastScreenView ? transformSessionToEvent(lastScreenView) : null, + session ? transformSessionToEvent(session) : null, ...eventsInDb, ]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/api/src/controllers/track.controller.ts(4 hunks)apps/api/src/utils/auth.ts(1 hunks)apps/public/content/docs/get-started/revenue-tracking.mdx(1 hunks)apps/worker/src/jobs/events.create-session-end.ts(5 hunks)packages/db/src/buffers/session-buffer.ts(5 hunks)packages/sdks/sdk/src/index.ts(1 hunks)packages/sdks/web/src/index.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/public/content/docs/get-started/revenue-tracking.mdx
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/sdks/sdk/src/index.ts
- packages/sdks/web/src/index.ts
- apps/api/src/utils/auth.ts
🧰 Additional context used
🧬 Code graph analysis (2)
apps/api/src/controllers/track.controller.ts (3)
packages/sdks/sdk/src/index.ts (1)
fetchDeviceId(199-206)packages/db/src/services/salt.service.ts (1)
getSalts(20-49)packages/redis/redis.ts (1)
getRedisCache(66-72)
apps/worker/src/jobs/events.create-session-end.ts (3)
packages/db/src/services/event.service.ts (3)
IServiceEvent(198-234)getEvents(310-353)transformSessionToEvent(103-150)packages/db/src/buffers/index.ts (1)
sessionBuffer(9-9)packages/db/src/clickhouse/client.ts (1)
convertClickhouseDateToJs(294-296)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-and-push-worker
- GitHub Check: build-and-push-dashboard
- GitHub Check: build-and-push-api
🔇 Additional comments (6)
packages/db/src/buffers/session-buffer.ts (3)
31-55: LGTM! Well-designed overload for dual-key session lookups.The discriminated union correctly enforces mutually exclusive lookup patterns (by sessionId OR by projectId+profileId), and the implementation properly dispatches to the appropriate Redis key. This enables both direct session lookups and profile-scoped session retrieval.
98-100: Revenue accumulation correctly handles legacy sessions.The explicit
?? 0defaults prevent NaN whennewSession.revenueis undefined for sessions cached before this rollout. This directly addresses the previous critical issue.
197-204: Profile-scoped cache correctly guarded.The
if (newSession.profile_id)guard prevents writing to malformed keys likesession:${projectId}:undefinedfor anonymous users. This resolves the previous major issue regarding session collision.apps/worker/src/jobs/events.create-session-end.ts (1)
95-109: Session property access is safe and correct.The event creation properly sources data from the session object (properties, exit_path, ended_at, profile_id) with appropriate null handling. The null check on lines 79-81 ensures
sessionis non-null for all subsequent accesses.apps/api/src/controllers/track.controller.ts (2)
9-9: LGTM!The import is correctly added and used in the new
fetchDeviceIdfunction.
413-434: The error is already logged at ERROR level; this review mischaracterizes the implementation.The catch block uses
request.log.error(), which logs at ERROR severity—not a lower level. The error is not masked; it's explicitly captured and logged. Additionally, after the error is logged, the function gracefully returns a 200 status with a defaultcurrentDeviceId, which appears to be the intended resilience strategy for this endpoint (device ID lookup can continue safely without Redis data).This pattern—logging errors and gracefully degrading—is consistent across the codebase (e.g.,
session-consistency.ts,base-buffer.ts). While system-wide metrics/monitoring integration would improve observability, that's not a specific actionable fix for this code, and the review does not identify an actual critical failure being masked.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/start/src/components/report-chart/common/report-table.tsx (1)
22-22: Remove unused import.The
logDependenciesimport frommathjsis not used anywhere in this file.Apply this diff:
-import { logDependencies } from 'mathjs';
🧹 Nitpick comments (4)
apps/start/src/routes/_app.$organizationId.$projectId.profiles.$profileId._tabs.events.tsx (1)
8-25: Profile events tab head/meta wiring looks goodThe new title import and
headconfiguration correctly usecreateProjectTitle(PAGE_TITLES.PROFILE_EVENTS)and align with the pattern in the profile details tab. If you later have easy access to project/organization names at this level, you could optionally pass them intocreateProjectTitlefor more descriptive titles, but this is not required for correctness.apps/start/src/routes/_app.$organizationId.$projectId.profiles.$profileId._tabs.index.tsx (1)
10-56: Profile details tab title configuration is consistent and correctUsing
createProjectTitle(PAGE_TITLES.PROFILE_DETAILS)inheadmatches the new title helper pattern and keeps route titles consistent across profile tabs. As an optional enhancement, if you later surfaceprojectName/organizationNamehere, passing them intocreateProjectTitlewould make the document title even clearer.apps/start/src/components/overview/overview-widget-table.tsx (1)
11-46: Consider extracting the hardcoded revenue color to a design token.The revenue color
#3ba974is hardcoded in both the SVG (line 36) and the text styling (lines 246, 417). For consistency and maintainability, consider extracting this to a CSS variable or design token that can be referenced throughout the application.Example approach:
<circle cx={size / 2} cy={size / 2} r={radius} fill="none" - stroke="#3ba974" + stroke="var(--color-revenue)" strokeWidth={strokeWidth} strokeDasharray={circumference} strokeDashoffset={offset} strokeLinecap="round" transform={`rotate(-90 ${size / 2} ${size / 2})`} className="transition-all" />apps/start/src/components/profiles/profile-metrics.tsx (1)
9-101: Hide-on-zero behavior and Revenue metric wiring look correctThe
hideOnZeroflag combined with the filter correctly hides only metrics that explicitly opt in (currently Revenue) when their value is exactly0, while leaving all other metrics unaffected. The newRevenuedefinition is consistent withIProfileMetrics.revenueand the'currency'unit used downstream.If you want to tighten things up later, you could slightly simplify the filter:
PROFILE_METRICS.filter( (metric) => !(metric.hideOnZero && data[metric.key] === 0), )but the current implementation is already clear and correct.
Also applies to: 107-112
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
apps/start/src/components/overview/overview-metric-card.tsx(6 hunks)apps/start/src/components/overview/overview-metrics.tsx(17 hunks)apps/start/src/components/overview/overview-widget-table.tsx(7 hunks)apps/start/src/components/profiles/profile-metrics.tsx(1 hunks)apps/start/src/components/report-chart/common/axis.tsx(2 hunks)apps/start/src/components/report-chart/common/report-chart-tooltip.tsx(1 hunks)apps/start/src/components/report-chart/common/report-table.tsx(1 hunks)apps/start/src/components/report-chart/conversion/chart.tsx(2 hunks)apps/start/src/hooks/use-format-date-interval.ts(2 hunks)apps/start/src/routes/_app.$organizationId.$projectId.profiles.$profileId._tabs.events.tsx(2 hunks)apps/start/src/routes/_app.$organizationId.$projectId.profiles.$profileId._tabs.index.tsx(2 hunks)apps/start/src/utils/title.ts(1 hunks)packages/db/src/services/profile.service.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/start/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/start/.cursorrules)
apps/start/**/*.{ts,tsx}: Instrument server functions created with createServerFn by wrapping their implementation in Sentry.startSpan and importing Sentry from '@sentry/tanstackstart-react'
When using Sentry in the TanStack Start React app, import it as: import * as Sentry from '@sentry/tanstackstart-react'
Files:
apps/start/src/components/report-chart/common/report-chart-tooltip.tsxapps/start/src/components/report-chart/conversion/chart.tsxapps/start/src/routes/_app.$organizationId.$projectId.profiles.$profileId._tabs.events.tsxapps/start/src/components/report-chart/common/axis.tsxapps/start/src/utils/title.tsapps/start/src/routes/_app.$organizationId.$projectId.profiles.$profileId._tabs.index.tsxapps/start/src/components/report-chart/common/report-table.tsxapps/start/src/hooks/use-format-date-interval.tsapps/start/src/components/overview/overview-metrics.tsxapps/start/src/components/overview/overview-metric-card.tsxapps/start/src/components/overview/overview-widget-table.tsxapps/start/src/components/profiles/profile-metrics.tsx
🧠 Learnings (3)
📚 Learning: 2025-10-16T10:29:28.924Z
Learnt from: CR
Repo: Openpanel-dev/openpanel PR: 0
File: apps/start/.cursorrules:0-0
Timestamp: 2025-10-16T10:29:28.924Z
Learning: Applies to apps/start/routes/**/*.tsx : Define React route files with createFileRoute and let the bundler/CLI manage the path string
Applied to files:
apps/start/src/routes/_app.$organizationId.$projectId.profiles.$profileId._tabs.events.tsx
📚 Learning: 2025-10-16T10:29:28.924Z
Learnt from: CR
Repo: Openpanel-dev/openpanel PR: 0
File: apps/start/.cursorrules:0-0
Timestamp: 2025-10-16T10:29:28.924Z
Learning: Applies to apps/start/routes/**/{index,*.index}.tsx : For index routes, use index.tsx or *.index.tsx files and ensure the createFileRoute path uses a trailing slash (e.g., '/posts/')
Applied to files:
apps/start/src/routes/_app.$organizationId.$projectId.profiles.$profileId._tabs.events.tsx
📚 Learning: 2025-10-16T10:29:28.924Z
Learnt from: CR
Repo: Openpanel-dev/openpanel PR: 0
File: apps/start/.cursorrules:0-0
Timestamp: 2025-10-16T10:29:28.924Z
Learning: Applies to apps/start/routes/**/_*.tsx : Use leading underscore filenames for pathless layout routes that wrap children without matching a URL segment
Applied to files:
apps/start/src/routes/_app.$organizationId.$projectId.profiles.$profileId._tabs.events.tsx
🧬 Code graph analysis (11)
apps/start/src/components/report-chart/common/report-chart-tooltip.tsx (1)
apps/start/src/hooks/use-format-date-interval.ts (1)
useFormatDateInterval(49-58)
packages/db/src/services/profile.service.ts (1)
packages/db/src/clickhouse/client.ts (1)
TABLE_NAMES(49-61)
apps/start/src/components/report-chart/conversion/chart.tsx (2)
apps/start/src/utils/date.ts (1)
formatDate(19-27)apps/start/src/hooks/use-format-date-interval.ts (1)
useFormatDateInterval(49-58)
apps/start/src/routes/_app.$organizationId.$projectId.profiles.$profileId._tabs.events.tsx (1)
apps/start/src/utils/title.ts (2)
createProjectTitle(39-53)PAGE_TITLES(78-128)
apps/start/src/components/report-chart/common/axis.tsx (2)
apps/start/src/utils/date.ts (1)
formatDate(19-27)apps/start/src/hooks/use-format-date-interval.ts (1)
useFormatDateInterval(49-58)
apps/start/src/routes/_app.$organizationId.$projectId.profiles.$profileId._tabs.index.tsx (1)
apps/start/src/utils/title.ts (2)
createProjectTitle(39-53)PAGE_TITLES(78-128)
apps/start/src/components/report-chart/common/report-table.tsx (1)
apps/start/src/hooks/use-format-date-interval.ts (1)
useFormatDateInterval(49-58)
apps/start/src/hooks/use-format-date-interval.ts (1)
packages/validation/src/types.validation.ts (1)
IInterval(34-34)
apps/start/src/components/overview/overview-metrics.tsx (6)
packages/validation/src/types.validation.ts (1)
IInterval(34-34)apps/start/src/hooks/use-format-date-interval.ts (1)
useFormatDateInterval(49-58)apps/start/src/hooks/use-numer-formatter.ts (1)
useNumber(73-108)apps/start/src/utils/theme.ts (1)
getChartColor(25-27)packages/common/src/get-previous-metric.ts (1)
getPreviousMetric(7-39)apps/start/src/components/report-chart/common/axis.tsx (1)
useYAxisProps(26-51)
apps/start/src/components/overview/overview-metric-card.tsx (3)
apps/start/src/hooks/use-numer-formatter.ts (1)
useNumber(73-108)apps/start/src/utils/date.ts (1)
formatDate(19-27)apps/public/components/ui/tooltip.tsx (1)
Tooltiper(36-51)
apps/start/src/components/overview/overview-widget-table.tsx (1)
apps/start/src/hooks/use-numer-formatter.ts (1)
useNumber(73-108)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-and-push-dashboard
- GitHub Check: build-and-push-worker
- GitHub Check: build-and-push-api
🔇 Additional comments (14)
apps/start/src/utils/title.ts (1)
93-96: PAGE_TITLES profile entries added cleanlyThe new
PROFILES,PROFILE_EVENTS, andPROFILE_DETAILSkeys fit the existing PAGE_TITLES structure and line up with their usage in the profile routes. No issues from a typing or behavior perspective.apps/start/src/components/report-chart/common/axis.tsx (1)
73-76: LGTM!The date formatting correctly adopts the new options object signature with
short: truefor concise axis labels.apps/start/src/components/report-chart/common/report-chart-tooltip.tsx (1)
65-68: LGTM!The date formatting correctly adopts the new options object signature with
short: falsefor full date display in tooltips.apps/start/src/components/report-chart/conversion/chart.tsx (2)
1-1: LGTM!The chart click handler is properly implemented with
useCallback, extracts the date from the clicked data point, and opens the AddReference modal. The integration is clean and follows React best practices.Also applies to: 5-5, 80-89, 95-95
174-177: LGTM!The date formatting correctly adopts the new options object signature with
short: falsefor full date display in the conversion tooltip.apps/start/src/components/report-chart/common/report-table.tsx (1)
43-46: LGTM!The date formatting correctly adopts the new options object signature with
short: truefor concise table column headers.apps/start/src/hooks/use-format-date-interval.ts (2)
3-20: LGTM!The refactor to an options object is clean and the
shortflag logic is correct: whenshortis false for hour/minute intervals, it includes month and day for context, otherwise just shows time. This provides appropriate formatting for both compact axis labels and verbose tooltips.
49-57: LGTM!The hook correctly forwards the options object to
formatDateIntervaland properly normalizes string/Date inputs. The refactor maintains a clean, composable API.apps/start/src/components/overview/overview-widget-table.tsx (3)
233-258: Revenue column implementation looks good.The conditional rendering based on
hasRevenueprevents displaying empty revenue columns, and the division by 100 correctly converts from cents to currency units. The percentage calculation is properly guarded against division by zero.
176-262: No issues found — responsive priorities are correctly assigned.The WidgetTable component documentation confirms that lower priority numbers indicate higher visibility priority (Path: 1 always visible, Sessions: 2, Revenue: 3, BR: 6, Duration: 7 hidden at smaller widths). The responsive priority assignments in overview-widget-table.tsx follow this semantic correctly, with the most important columns prioritized first.
419-420: The currency formatter supports the options parameter—no changes needed.Verification confirms that the
currency()function returned byformatCurrencyaccepts an optionaloptionsparameter with ashortproperty. Line 420's call tonumber.currency(revenue / 100, { short: true })is correct and consistent with the API, and similar patterns are already used elsewhere in the codebase (e.g.,apps/start/src/components/projects/project-card.tsx:145).packages/db/src/services/profile.service.ts (1)
18-32: Revenue metric plumbing is consistent; confirm stored unit (cents vs. major currency)Adding
revenue: numbertoIProfileMetricsand sourcing it via the newrevenueCTE (summingevents.revenuewherename = 'revenue') lines up with how the UI expects to consume it, and the ClickHouse CTE structure matches the existing pattern.The front end is formatting revenue by dividing by
100before passing values intonumber.currency, which implies thatevents.revenueis stored in cents (or another smallest currency unit). Please double‑check that the column and ingestion logic indeed use that convention; if the DB stores major units already, the/ 100in the UI should be dropped instead.Also applies to: 80-83, 96-97
apps/start/src/components/overview/overview-metrics.tsx (2)
41-84: Revenue title and per-metric data wiring are consistentAdding the
Revenueentry toTITLESwith keytotal_revenueand unit'currency', and feedingOverviewMetricCardwith{ date, current: item[title.key], previous: item[\prev_${title.key}`] }` keeps the overview metrics panel aligned with the stats API shape.This sets up the card correctly for both the main value and the per-point sparkline/tooltip behavior for revenue (in cents, formatted as currency in the card).
Also applies to: 114-133
166-173: Tooltip currency formatting and Revenue block behavior are coherentThe tooltip implementation:
- Uses
useFormatDateIntervalwith the providedintervalfor the header date.- Correctly formats currency metrics as
number.currency((data[metric.key] ?? 0) / 100), and does the same for previous values.- Adds a separate Revenue section when
anyMetric && revenue > 0, formatting both current and previous revenue in dollars and feeding them intoPreviousDiffIndicatorPureviagetPreviousMetric.This matches the rest of the app’s “cents in data, divide by 100 in UI” convention and cleanly separates the active metric from the revenue overlay.
Please just confirm the backend always supplies
total_revenue/prev_total_revenuein cents so these/ 100conversions stay correct across all charts.Also applies to: 175-179, 185-187, 197-197, 203-216, 229-254
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/db/code-migrations/6-add-revenue-column.ts (2)
9-18: Document the revenue data type and units.The migration adds a
revenuecolumn with typeUInt64, which is appropriate for storing integer values. However, the units and precision are not documented:
- If revenue is stored in cents (or smallest currency unit), this should be documented in a comment
- If revenue needs to support decimal values (e.g., $10.99),
UInt64won't work and you'd needDecimal64(2)or similar- The column will default to
0for existing rows, which is likely correct but worth confirmingConsider adding a comment to clarify:
const sqls: string[] = [ ...addColumns( 'events', + // Revenue stored as integer in smallest currency unit (e.g., cents for USD) ['`revenue` UInt64 AFTER `referrer_type`'], isClustered, ), ];
33-35: LGTM! Consider adding a rollback function.The migration execution logic is correct and includes dry-run support, which is excellent for testing.
However, consider adding a
down()function to support rollback:export async function down() { const isClustered = getIsCluster(); const sqls: string[] = dropColumns('events', ['revenue'], isClustered); if (!process.argv.includes('--dry')) { await runClickhouseMigrationCommands(sqls); } }This would allow reverting the migration if needed during development or troubleshooting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.gitignore(1 hunks)packages/db/code-migrations/3-init-ch.sql(0 hunks)packages/db/code-migrations/5-add-imports-table.sql(0 hunks)packages/db/code-migrations/6-add-revenue-column.ts(1 hunks)packages/db/src/clickhouse/migration.ts(1 hunks)
💤 Files with no reviewable changes (2)
- packages/db/code-migrations/5-add-imports-table.sql
- packages/db/code-migrations/3-init-ch.sql
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-26T18:30:17.476Z
Learnt from: CR
Repo: Openpanel-dev/openpanel PR: 0
File: .cursorrules:0-0
Timestamp: 2025-09-26T18:30:17.476Z
Learning: When writing ClickHouse queries, use the helper functions in ./packages/db/src/clickhouse/query-functions.ts rather than crafting raw queries
Applied to files:
packages/db/code-migrations/6-add-revenue-column.ts
🧬 Code graph analysis (1)
packages/db/code-migrations/6-add-revenue-column.ts (2)
packages/db/code-migrations/helpers.ts (1)
getIsCluster(17-24)packages/db/src/clickhouse/migration.ts (2)
addColumns(137-152)runClickhouseMigrationCommands(338-470)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-and-push-dashboard
- GitHub Check: build-and-push-api
- GitHub Check: build-and-push-worker
🔇 Additional comments (1)
packages/db/code-migrations/6-add-revenue-column.ts (1)
20-31: LGTM!Writing the SQL statements to a
.sqlfile is good practice for documentation and debugging. The formatting logic correctly normalizes the statements and uses a clear delimiter.
1 participant
Summary by CodeRabbit
New Features
Documentation
UI Improvements