Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughIntroduces a comprehensive revenue tracking system integrating Paddle and Stripe payment providers, enabling transaction synchronization, revenue analytics, and encrypted API key storage. Adds new backend services, controllers, and adapters; extends analytics to include revenue data in profiles and sessions; updates database schemas; and provides frontend UI for revenue management and data visualization. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Browser
participant RevenueController
participant RevenueService
participant PaddleAdapter
participant PaddleAPI
participant ClickHouse
participant Project DB
User->>Browser: Connect Paddle (API key + currency)
Browser->>RevenueController: POST /connect-revenue
RevenueController->>PaddleAdapter: validateApiKey(apiKey)
PaddleAdapter->>PaddleAPI: Test API call
PaddleAPI-->>PaddleAdapter: ✓ Valid
RevenueController->>RevenueService: connectPaddle(projectId, key, currency)
RevenueService->>RevenueService: encrypt(apiKey)
RevenueService->>Project DB: Update project with<br/>paddleApiKeyEnc, currency
Project DB-->>RevenueService: ✓
RevenueController-->>Browser: { success: true }
Note over Browser,ClickHouse: Scheduled Sync (every 30 min)
participant TaskManager
TaskManager->>TaskManager: cron trigger
TaskManager->>RevenueService: getPaddleApiKey(projectId)
RevenueService-->>TaskManager: decrypted apiKey
TaskManager->>PaddleAdapter: syncTransactions(projectId, key, currency)
PaddleAdapter->>PaddleAPI: Fetch transactions (paginated)
PaddleAPI-->>PaddleAdapter: Transaction list
PaddleAdapter->>PaddleAdapter: Process & map to<br/>RevenueTransaction
PaddleAdapter->>RevenueService: insertTransaction(tx)
RevenueService->>ClickHouse: INSERT INTO revenue
ClickHouse-->>RevenueService: ✓
RevenueService->>Project DB: Update revenueLastSyncAt
PaddleAdapter-->>TaskManager: ✓ Sync complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Areas requiring extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
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 |
|
@CodeRabbit please review |
|
@Blaumaus: I'll review the changes in this PR now. ✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/apps/cloud/src/analytics/interfaces/index.ts (1)
183-188: Fix type safety mismatch in processPageflow return type.The
processPageflow()method in analytics.service.ts returns objects withamountandcurrencyfields for 'sale' and 'refund' event types (extracted from metadata), but the method has no explicit return type annotation. This creates a type safety gap: the actual return type doesn't match theIPageflowinterface signature.For sale/refund events, the method extracts amount and currency from the metadata array and adds them as explicit object properties. However,
IPageflowdoesn't declare these fields, causing a contract violation between the declared input type and actual output structure.Recommendation: Define an explicit return type for
processPageflow()that accounts for the optionalamountandcurrencyfields on sale/refund events, or extendIPageflowto include these fields as optional properties.
🧹 Nitpick comments (31)
backend/apps/community/src/project/project.controller.ts (1)
201-228: Consider using a Set for O(1) lookups in the sort comparator.The
_includes(pinnedProjectIds, ...)call inside the sort comparator runs O(n log n) times, each with O(m) array lookup. For better performance, convertpinnedProjectIdsto a Set before sorting.🔎 Proposed optimization
// Get pinned project IDs to mark shared projects as pinned too const pinnedProjectIds = await getPinnedProjectsClickhouse(userId) + const pinnedProjectIdsSet = new Set(pinnedProjectIds) // Always sort by pinned first, then by the requested sort criteria combinedProjects = [...combinedProjects].sort((a: any, b: any) => { // First, sort by pinned status (pinned projects first) - const aIsPinned = a.isPinned || _includes(pinnedProjectIds, a.id) - const bIsPinned = b.isPinned || _includes(pinnedProjectIds, b.id) + const aIsPinned = a.isPinned || pinnedProjectIdsSet.has(a.id) + const bIsPinned = b.isPinned || pinnedProjectIdsSet.has(b.id)backend/migrations/clickhouse/2025_12_15_revenue.js (2)
30-30: Consider handling the migration result.
queriesRunnerreturns a boolean indicating success/failure, but the result is not being checked. For consistency with other migrations, consider awaiting and logging the result:🔎 Suggested improvement
-queriesRunner(queries) +const run = async () => { + const success = await queriesRunner(queries) + if (!success) { + process.exit(1) + } +} + +run()
3-28: Schema duplication with initialise_database.js.This table schema is duplicated in
backend/migrations/clickhouse/initialise_database.js(lines 209-233). While this is common for migrations, be mindful that future schema changes need to be applied in both locations to maintain consistency for new deployments..claude/frontend-design.md (1)
1-45: LGTM - Good addition of design guidelines.This document provides valuable guidance for maintaining consistent, high-quality frontend aesthetics. The guidelines around typography, color, motion, and avoiding generic patterns are well-articulated.
One minor note: Line 45 uses "thinking outside the box" which could be rephrased to align with the document's own advice about avoiding clichéd expressions.
web/app/pages/Project/Traffic/View/TrafficView.tsx (1)
182-182: Unused state variablerevenueChartData.The state
revenueChartDatais declared and set on line 641, but it's never read after that. The revenue data is instead embedded directly intochartWithRevenueand passed tosetChartData. Consider removing this unused state to reduce complexity.Suggested fix
- const [revenueChartData, setRevenueChartData] = useState<number[]>([])And remove line 641:
- setRevenueChartData(revenueData)backend/apps/cloud/src/revenue/dto/get-revenue.dto.ts (1)
12-42: Consider adding validation forperiodandtimezonefields.The
periodfield accepts any string, but it likely has a finite set of valid values (e.g.,7d,30d,custom). Similarly,timezoneshould be a valid IANA timezone. Consider adding validation to catch invalid inputs early.Optional: Add stricter validation
import { IsIn, IsTimeZone } from 'class-validator' // For period (if you have a known set of values): @IsIn(['7d', '30d', '90d', 'custom', 'all']) period: string // For timezone (class-validator has built-in timezone validation): @IsOptional() @IsTimeZone() timezone?: stringbackend/apps/cloud/src/analytics/analytics.module.ts (1)
12-12: Consider refactoring to avoid circular dependency.AnalyticsModule and RevenueModule have a bidirectional circular dependency (both use
forwardRefto import each other). WhileforwardRefresolves this at runtime, circular dependencies can indicate architectural coupling issues and make the codebase harder to maintain and test.Consider:
- Extracting shared interfaces/DTOs into a separate common module
- Restructuring so only one module depends on the other (e.g., AnalyticsModule → RevenueModule, but not the reverse)
- Using events or a mediator pattern to decouple the modules
If the circular dependency is intentional and necessary for the revenue analytics integration, add a comment explaining why both modules need to reference each other.
Also applies to: 20-20
web/app/api/index.ts (1)
2470-2494: Consider whether USD default is the appropriate choice for currency initialization.The
currencyparameter defaults to'USD'in the frontend function and backend endpoint (via the optional DTO field). While the UI allows users to explicitly select a currency, consider:
- Whether this default should be inferred from
project.revenueCurrencywhen already set- Whether requiring explicit currency selection (removing the default) would improve clarity
- Documenting that USD is the fallback assumption when no currency is specified
The project entity already supports per-project currency tracking, so the infrastructure exists to make this decision automatic rather than defaulting to USD globally.
web/app/styles/ProjectViewStyle.css (1)
113-122: Consider using CSS variables for color consistency.The revenue chart styling uses a hardcoded hex color
#ea580cwhile most other target selectors (e.g.,.bb-target-unique,.bb-target-total) use CSS variables likevar(--color-amber-600). For maintainability and theme consistency, consider using a CSS variable.Additionally, the revenue and refunds selectors are missing the
!importantflag on thestrokeproperty that other targets have. Verify this is intentional based on the cascade behavior you need.🔎 Suggested change for consistency
/* Revenue chart styling */ .bb-target-revenue .bb-bar { - stroke: #ea580c; + stroke: var(--color-orange-600) !important; fill-opacity: 0.85 !important; } .bb-target-refundsAmount .bb-bar { - stroke: #ea580c; + stroke: var(--color-orange-600) !important; fill-opacity: 1 !important; /* keep rgba() fill visible (don't multiply by .bb-bar opacity) */ }backend/migrations/mysql/2025_12_15_revenue.sql (1)
1-5: Consider combining ALTER TABLE statements for better performance.On large tables, multiple
ALTER TABLEstatements can be slow as MySQL may rebuild the table for each statement. Consider combining them into a single statement for better migration performance.🔎 Suggested optimization
-- Revenue / Payment provider integration fields for Project table -ALTER TABLE `project` ADD COLUMN `paddleApiKeyEnc` text DEFAULT NULL; -ALTER TABLE `project` ADD COLUMN `revenueCurrency` varchar(3) DEFAULT NULL; -ALTER TABLE `project` ADD COLUMN `paddleApiKeyPermissions` text DEFAULT NULL; -ALTER TABLE `project` ADD COLUMN `revenueLastSyncAt` datetime DEFAULT NULL; +ALTER TABLE `project` + ADD COLUMN `paddleApiKeyEnc` text DEFAULT NULL, + ADD COLUMN `revenueCurrency` varchar(3) DEFAULT NULL, + ADD COLUMN `paddleApiKeyPermissions` text DEFAULT NULL, + ADD COLUMN `revenueLastSyncAt` datetime DEFAULT NULL;Additionally, if queries will filter or sort by
revenueLastSyncAt(e.g., finding projects due for sync), consider adding an index:ALTER TABLE `project` ADD INDEX `idx_project_revenue_last_sync` (`revenueLastSyncAt`);web/app/pages/Project/View/components/UserDetails.tsx (1)
284-294: Minor inconsistency in currency source.The revenue
StatItemusesdetails.revenueCurrencyfor currency formatting, while theSessionscomponent receivescurrencyfrom the parent prop. This creates two different currency sources within the same component.If both should use the same currency (from the project), consider using the
currencyprop consistently:🔎 Suggested change
{details.totalRevenue !== undefined && details.totalRevenue > 0 ? ( <StatItem icon={<DollarSignIcon className='h-3.5 w-3.5 text-green-500' />} label={t('dashboard.revenue')} value={new Intl.NumberFormat('en-US', { style: 'currency', - currency: details.revenueCurrency || 'USD', + currency: currency || details.revenueCurrency || 'USD', minimumFractionDigits: 2, }).format(details.totalRevenue)} /> ) : null}Alternatively, if
details.revenueCurrencyis the correct source for profile-level data whilecurrencyis for session-level display, document this distinction.web/app/pages/Project/Settings/tabs/Revenue.tsx (1)
131-135: Consider improving error message extraction.The error check
typeof error === 'string'may not capture API error messages if they come as objects (e.g.,{ message: '...' }). This pattern is repeated inhandleDisconnectandhandleCurrencyChangeas well.🔎 Suggested helper for consistent error handling
// Could extract to a utility function const getErrorMessage = (error: unknown, fallback: string): string => { if (typeof error === 'string') return error if (error && typeof error === 'object' && 'message' in error && typeof error.message === 'string') { return error.message } return fallback } // Usage: toast.error(getErrorMessage(error, t('apiNotifications.somethingWentWrong')))backend/apps/cloud/src/revenue/dto/payment-log.dto.ts (1)
1-52: DTOs are solid; consider tightening currency validationDTO shapes and basic validation look good for the new log endpoints. To avoid bad currency codes slipping through, you could strengthen the
currencyfield validation:
- Enforce 3‑letter codes.
- Enforce uppercase.
Example:
Suggested currency validation refinement
-import { IsNotEmpty, IsString, IsOptional, IsNumber } from 'class-validator' +import { IsNotEmpty, IsString, IsOptional, IsNumber, Length, IsUppercase } from 'class-validator' @ApiPropertyOptional({ description: 'Currency code' }) @IsOptional() - @IsString() + @IsString() + @Length(3, 3) + @IsUppercase() currency?: stringThis keeps the endpoint flexible while ensuring inputs match expected ISO-style codes.
backend/apps/cloud/src/task-manager/task-manager.service.ts (1)
62-63: Revenue sync cron is well-integrated; consider throttling and idempotency assumptionsThe new
syncRevenueDatajob is wired correctly into DI and filters projects sensibly (non-nullpaddleApiKeyEnc, paid/non-blocked admin). Error handling and per‑project logging look good.Two non-blocking points to consider:
Concurrency vs Paddle rate limits
Promise.allSettled(_map(projects, ...))will hit Paddle with one concurrent sync per project. If you grow to many Paddle-connected projects, this could stress your rate limits. You already have amapLimithelper above; reusing it here would cap concurrency, e.g.:Example using existing
mapLimithelper
- const promises = _map(projects, async project => {
try {const apiKey = this.revenueService.getPaddleApiKey(project)...} catch (error) {this.logger.error(`[CRON WORKER](syncRevenueData) Error syncing project ${project.id}: ${error}`,)}- })
- await Promise.allSettled(promises).catch(reason => {
- await mapLimit(projects, 5, async project => {
try {const apiKey = this.revenueService.getPaddleApiKey(project)...} catch (error) {this.logger.error(`[CRON WORKER](syncRevenueData) Error syncing project ${project.id}: ${error}`,)}- }).catch(reason => {
this.logger.error(
[CRON WORKER](syncRevenueData) Error occured: ${reason},
)
})</details> 2. **Multi-instance cron execution** If the cloud app runs multiple instances, each will execute this cron. Make sure `paddleAdapter.syncTransactions` is idempotent (e.g. de-duping on `transactionId` at DB level) so double-runs don’t create duplicate records or inconsistent aggregates. Also applies to: 328-329, 1083-1132 </blockquote></details> <details> <summary>web/app/pages/Project/View/ViewProject.helpers.tsx (1)</summary><blockquote> `21-21`: **Revenue/refunds integration is well-wired; watch axis units and hard-coded “$”** The revenue series wiring (columns, types, stacking with `refundsAmount`, colors, legend behaviour, and `onrendered` styling) is coherent and matches the intended “net revenue + dashed refunds overlay” design. A few non-blocking concerns: 1. **Hard-coded USD in axis and tooltip** - `y2.tick.format` and the new tooltip branches format values as `$${nFormatter(...)}`. - Projects can configure a reporting currency; sessions UI already respects `revenueCurrency`, but this chart will always show `$`. It would be more accurate to thread a `currencySymbol` into `getSettings` and use it instead of a literal `$`. 2. **Mixed units on `y2` axis** `y2` now potentially carries **bounce (%)**, **session duration (time)**, and **revenue (currency)**: ```ts axes: { bounce: 'y2', sessionDuration: 'y2', revenue: 'y2', refundsAmount: 'y2', } ... show: activeChartMetrics.bounce || activeChartMetrics.sessionDuration || activeChartMetrics.revenue tick.format: activeChartMetrics.bounce ? d => `${d}%` : activeChartMetrics.revenue ? d => `$${nFormatter(d, 1)}` : d => getStringFromTime(...)If users enable bounce + revenue (or sessionDuration + revenue) together, the axis unit and min/max (10–100 for bounce) no longer match all attached series and can clip revenue. Either gating these metrics to be mutually exclusive or separating revenue onto its own axis would avoid confusing scales.
Reliance on billboard.js internals
The
onrenderedhook manipulateschart.$.bar.barsdirectly:// @ts-expect-error const chart = this as any if (chart?.$ && chart.$.bar?.bars) { chart.$.bar.bars.each(function (this: SVGPathElement, d: any) { ... }) }That works today but depends on private/internal fields that may change between billboard.js versions. It’s worth double-checking against your pinned billboard version and re-verifying on upgrades.
Also applies to: 236-236, 329-339, 633-667, 708-720, 755-778, 889-899, 900-939
web/app/pages/Project/View/components/Sessions.tsx (1)
29-40: Session-level revenue display is clear; improve currency fallbackThe revenue display per session is nicely integrated (sign-aware, colored, and used in both compact and expanded layouts), and threading
currencythroughSessions→Sessionkeeps the API tidy.One thing to adjust:
getCurrencySymbolcurrently falls back to USD even for unknown codes:const getCurrencySymbol = (currency?: string) => { return CURRENCY_SYMBOLS[currency || 'USD'] || CURRENCY_SYMBOLS.USD }If the backend ever sends, say,
JPYorCHF, this will silently render them as$…. A safer approach is to:
- Use USD only when
currencyis omitted, and- For unknown but present codes, fall back to the code itself (or no symbol).
For example:
Suggested safer currency fallback
const getCurrencySymbol = (currency?: string) => {return CURRENCY_SYMBOLS[currency || 'USD'] || CURRENCY_SYMBOLS.USD
- if (!currency) {
- return CURRENCY_SYMBOLS.USD
- }
- return CURRENCY_SYMBOLS[currency] ??
${currency}
}</details> This avoids mislabeling while still giving a sensible default when no currency is configured. Also applies to: 65-66, 73-74, 82-83, 211-223, 243-255, 279-285, 303-304 </blockquote></details> <details> <summary>backend/apps/cloud/src/revenue/interfaces/revenue.interface.ts (2)</summary><blockquote> `1-6`: **Consider removing unused enum values or marking them as planned.** `POLAR` and `LEMONSQUEEZY` are defined in `RevenueProvider` but don't appear to be used anywhere in this PR. If these are planned for future implementation, consider adding a comment. Otherwise, remove them to avoid confusion. --- `75-76`: **Production URL change required before merge.** The TODO indicates this sandbox URL must be changed to `vendors.paddle.com` for production. This should be tracked to ensure it's not forgotten. Would you like me to open an issue to track this production URL change? </blockquote></details> <details> <summary>backend/apps/cloud/src/revenue/adapters/stripe.adapter.ts (3)</summary><blockquote> `66-79`: **Add timeout to prevent indefinite hangs on API calls.** The `fetch` call has no timeout configured. If Stripe's API becomes unresponsive, this could block indefinitely. <details> <summary>🔎 Proposed fix using AbortController</summary> ```diff async validateApiKey(apiKey: string): Promise<boolean> { try { + const controller = new AbortController() + const timeoutId = setTimeout(() => controller.abort(), 10000) + const res = await fetch(`${this.baseUrl}/account`, { headers: { Authorization: `Bearer ${apiKey}`, }, + signal: controller.signal, }) + clearTimeout(timeoutId) return res.ok } catch (error) { this.logger.error({ error }, 'Failed to validate Stripe API key') return false } }
172-176: Apply same timeout pattern to paginated fetch calls.The
fetchAllmethod makes multiple API calls without timeouts. Each call should have a timeout to prevent sync jobs from hanging.
115-122: Sequential transaction processing may be slow for large datasets.Processing transactions one-by-one with
awaitin a loop can be slow when syncing many transactions. Consider batching inserts or usingPromise.allwith concurrency limits for better performance.Also applies to: 131-139
backend/apps/cloud/src/analytics/analytics.controller.ts (2)
2384-2384: Avoidas anycast - use proper typing.The cast
{ pid: dto.pid } as anybypasses type safety. Create a minimal interface or use a partial type instead.🔎 Proposed fix
- await this.analyticsService.validate({ pid: dto.pid } as any, origin, ip) + await this.analyticsService.validate({ pid: dto.pid } as Pick<PageviewsDto, 'pid'>, origin, ip)Or if
validateaccepts a broader type, update its signature to reflect that.
2308-2332: Profile/session ID endpoints look correct but should also have rate limiting.These public endpoints generate identifiers based on IP/UA fingerprinting. While less critical than payment logging, rate limiting would still be prudent to prevent enumeration attacks.
Also applies to: 2334-2369
backend/apps/cloud/src/revenue/adapters/paddle.adapter.ts (1)
87-101: Add timeouts to all fetch calls.Same concern as Stripe adapter - all
fetchcalls should have timeouts to prevent indefinite hangs during sync operations.Also applies to: 181-189, 239-247
web/app/pages/Project/View/components/Pageflow.tsx (1)
208-211: Use user's locale for currency formatting instead of hardcoded 'en-US'.The component already has access to
languagefromuseTranslation()but uses hardcoded'en-US'for currency formatting. This could display currencies incorrectly for non-US users (e.g.,$1,234.56vs1.234,56 $).🔎 Proposed fix
Since
PageflowItemdoesn't have direct access tolanguage, pass it as a prop or use a more appropriate locale:+ locale?: string } const PageflowItem = ({ ... + locale = 'en-US', }: PageflowItemProps) => { ... {new Intl.NumberFormat('en-US', { + {new Intl.NumberFormat(locale, { style: 'currency', currency: currency || 'USD', }).format(Math.abs(amount || 0))}And pass from parent:
<PageflowItem ... + locale={language} />
backend/apps/cloud/src/revenue/revenue.controller.ts (4)
165-193: Missing @apiresponse decorator for updateCurrency endpoint.Other endpoints in this controller have
@ApiResponsedecorators, butupdateCurrencyis missing one for consistency with the API documentation.🔎 Proposed fix
@ApiBearerAuth() @Post('/:pid/revenue/currency') @Auth() + @ApiResponse({ status: 200 }) async updateCurrency(
195-257: Missing @apiresponse decorator for syncRevenue endpoint.For API documentation consistency, add the
@ApiResponsedecorator.🔎 Proposed fix
@ApiBearerAuth() @Post('/:pid/revenue/sync') @Auth() + @ApiResponse({ status: 200 }) async syncRevenue(
291-295: Consider checking revenue connection status in analytics controller.The
getRevenueStatsendpoint correctly checks for revenue configuration, but this pattern should be consistent across all analytics endpoints. ThegetRevenueTransactionsandgetRevenueBreakdownendpoints don't have this check, which could lead to confusing empty results instead of a clear error message.🔎 Proposed fix for getRevenueTransactions
this.projectService.allowedToView(project, userId) + if (!project.paddleApiKeyEnc && !project.stripeApiKeyEnc) { + throw new BadRequestException( + 'Revenue tracking is not configured for this project', + ) + } + const safeTimezone = this.analyticsService.getSafeTimezone(dto.timezone)🔎 Proposed fix for getRevenueBreakdown
this.projectService.allowedToView(project, userId) + if (!project.paddleApiKeyEnc && !project.stripeApiKeyEnc) { + throw new BadRequestException( + 'Revenue tracking is not configured for this project', + ) + } + const safeTimezone = this.analyticsService.getSafeTimezone(dto.timezone)
305-315: Type assertionas anyfor timeBucket parameter.Multiple places cast
timeBuckettoanywhen passing togetGroupFromToandgenerateXAxis. Consider properly typing thetimeBucketvariable or updating the DTO to useTimeBucketType.
backend/apps/cloud/src/revenue/revenue.service.ts (2)
52-63: Add error handling for malformed encrypted data in decrypt method.The
decryptmethod will throw an unclear error if the encrypted text doesn't contain the expected:separator or has invalid hex encoding. Consider adding validation.🔎 Proposed fix
private decrypt(encryptedText: string): string { + if (!encryptedText || !encryptedText.includes(':')) { + throw new Error('Invalid encrypted data format') + } const [ivHex, encrypted] = encryptedText.split(':') + if (!ivHex || !encrypted || ivHex.length !== 32) { + throw new Error('Invalid encrypted data format') + } const iv = Buffer.from(ivHex, 'hex') const decipher = crypto.createDecipheriv( 'aes-256-cbc', this.encryptionKey, iv, ) let decrypted = decipher.update(encrypted, 'hex', 'utf8') decrypted += decipher.final('utf8') return decrypted }
629-701: Code duplication:getGroupSubqueryandgenerateDateStringare duplicated from AnalyticsService.These methods are nearly identical to the ones in
AnalyticsService. Consider extracting them to a shared utility module to maintain DRY principles.#!/bin/bash # Verify the duplication in AnalyticsService rg -n "getGroupSubquery|generateDateString" backend/apps/cloud/src/analytics/analytics.service.ts
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (45)
.claude/frontend-design.md(1 hunks)backend/apps/cloud/src/analytics/analytics.controller.ts(3 hunks)backend/apps/cloud/src/analytics/analytics.module.ts(1 hunks)backend/apps/cloud/src/analytics/analytics.service.ts(14 hunks)backend/apps/cloud/src/analytics/interfaces/index.ts(1 hunks)backend/apps/cloud/src/app.module.ts(2 hunks)backend/apps/cloud/src/project/entity/project.entity.ts(1 hunks)backend/apps/cloud/src/project/project.service.ts(3 hunks)backend/apps/cloud/src/revenue/adapters/paddle.adapter.ts(1 hunks)backend/apps/cloud/src/revenue/adapters/stripe.adapter.ts(1 hunks)backend/apps/cloud/src/revenue/dto/connect-paddle.dto.ts(1 hunks)backend/apps/cloud/src/revenue/dto/connect-revenue.dto.ts(1 hunks)backend/apps/cloud/src/revenue/dto/get-revenue.dto.ts(1 hunks)backend/apps/cloud/src/revenue/dto/payment-log.dto.ts(1 hunks)backend/apps/cloud/src/revenue/dto/revenue-stats.dto.ts(1 hunks)backend/apps/cloud/src/revenue/interfaces/revenue.interface.ts(1 hunks)backend/apps/cloud/src/revenue/revenue.controller.ts(1 hunks)backend/apps/cloud/src/revenue/revenue.module.ts(1 hunks)backend/apps/cloud/src/revenue/revenue.service.ts(1 hunks)backend/apps/cloud/src/task-manager/task-manager.module.ts(2 hunks)backend/apps/cloud/src/task-manager/task-manager.service.ts(3 hunks)backend/apps/community/src/common/utils.ts(2 hunks)backend/apps/community/src/project/project.controller.ts(2 hunks)backend/migrations/clickhouse/2025_12_15_revenue.js(1 hunks)backend/migrations/clickhouse/initialise_database.js(1 hunks)backend/migrations/mysql/2025_12_15_revenue.sql(1 hunks)backend/migrations/mysql/2025_12_18_revenue_stripe.sql(1 hunks)web/app/api/index.ts(1 hunks)web/app/lib/models/Project.ts(2 hunks)web/app/pages/Project/Profiles/View/ProfilesView.tsx(1 hunks)web/app/pages/Project/Sessions/View/SessionsView.tsx(1 hunks)web/app/pages/Project/Settings/ProjectSettings.tsx(5 hunks)web/app/pages/Project/Settings/tabs/DangerZone.tsx(3 hunks)web/app/pages/Project/Settings/tabs/Revenue.tsx(1 hunks)web/app/pages/Project/Traffic/View/TrafficView.tsx(7 hunks)web/app/pages/Project/View/Panels.tsx(4 hunks)web/app/pages/Project/View/ViewProject.helpers.tsx(8 hunks)web/app/pages/Project/View/ViewProject.tsx(1 hunks)web/app/pages/Project/View/components/Pageflow.tsx(9 hunks)web/app/pages/Project/View/components/Sessions.tsx(7 hunks)web/app/pages/Project/View/components/UserDetails.tsx(5 hunks)web/app/styles/ProjectViewStyle.css(2 hunks)web/app/ui/icons/Paddle.tsx(1 hunks)web/app/ui/icons/Stripe.tsx(1 hunks)web/public/locales/en.json(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (13)
backend/migrations/clickhouse/2025_12_15_revenue.js (2)
backend/migrations/clickhouse/initialise_database.js (1)
require(2-2)backend/migrations/clickhouse/setup.js (2)
dbName(107-107)queriesRunner(76-105)
web/app/pages/Project/Sessions/View/SessionsView.tsx (1)
web/app/pages/Project/View/components/Sessions.tsx (1)
Sessions(279-310)
backend/apps/cloud/src/revenue/adapters/stripe.adapter.ts (2)
backend/apps/cloud/src/revenue/interfaces/revenue.interface.ts (1)
RevenueTransaction(20-38)web/app/api/index.ts (1)
RevenueProvider(2470-2470)
backend/apps/community/src/project/project.controller.ts (1)
backend/apps/community/src/common/utils.ts (1)
getPinnedProjectsClickhouse(1272-1272)
web/app/pages/Project/View/components/Sessions.tsx (1)
web/app/lib/models/Project.ts (1)
Session(95-115)
backend/apps/cloud/src/revenue/revenue.module.ts (2)
backend/apps/cloud/src/analytics/analytics.module.ts (1)
Module(14-26)backend/apps/cloud/src/task-manager/task-manager.module.ts (1)
Module(20-40)
backend/apps/cloud/src/analytics/analytics.controller.ts (3)
backend/apps/cloud/src/revenue/dto/payment-log.dto.ts (3)
GetProfileIdDto(4-9)GetSessionIdDto(11-16)LogPaymentDto(18-52)backend/apps/cloud/src/common/utils.ts (1)
getIPFromHeaders(285-312)web/app/api/index.ts (2)
RevenueProvider(2470-2470)RevenueStatus(2436-2441)
web/app/pages/Project/View/ViewProject.helpers.tsx (2)
web/app/utils/generic.ts (3)
nFormatter(20-24)getStringFromTime(81-91)getTimeFromSeconds(64-79)web/fly_import_env.cjs (1)
value(14-14)
web/app/pages/Project/View/components/UserDetails.tsx (1)
web/app/pages/Project/View/components/Sessions.tsx (1)
Sessions(279-310)
backend/apps/cloud/src/revenue/revenue.service.ts (3)
backend/apps/cloud/src/revenue/adapters/stripe.adapter.ts (1)
Injectable(57-346)backend/apps/cloud/src/revenue/adapters/paddle.adapter.ts (1)
Injectable(77-387)backend/apps/cloud/src/revenue/interfaces/revenue.interface.ts (2)
PADDLE_REQUIRED_PERMISSIONS(67-73)RevenueTransaction(20-38)
backend/apps/cloud/src/analytics/analytics.module.ts (3)
backend/apps/cloud/src/revenue/revenue.module.ts (1)
Module(16-27)backend/apps/cloud/src/app.module.ts (1)
Module(97-111)backend/apps/cloud/src/task-manager/task-manager.module.ts (1)
Module(20-40)
backend/migrations/clickhouse/initialise_database.js (1)
backend/migrations/clickhouse/setup.js (1)
dbName(107-107)
web/app/pages/Project/Settings/ProjectSettings.tsx (1)
web/app/lib/constants/index.ts (1)
isSelfhosted(319-319)
🪛 LanguageTool
.claude/frontend-design.md
[style] ~45-~45: “thinking outside the box” can be a clichéd phrase in professional communication. Consider an alternative expression to make your writing more engaging and original.
Context: ...ck, show what can truly be created when thinking outside the box and committing fully to a distinctive v...
(THINK_OUTSIDE_THE_BOX)
🔇 Additional comments (50)
backend/apps/community/src/project/project.controller.ts (2)
294-305: LGTM!The
isPinnedflag is correctly computed for each project in the response payload, combining both the project's own flag and the pinned IDs lookup.
307-313: LGTM!The return structure correctly uses the pre-sorted results. The comment accurately documents that pinned sorting was already applied in the combined sort step above.
web/app/pages/Project/View/Panels.tsx (1)
716-716: Transparent sticky header may cause readability issues when scrolling.Changing the sticky header background from
bg-white dark:bg-slate-800/25tobg-transparentwill allow scrolling content to show through the header, potentially making the column labels difficult to read. The same change applies to lines 1120, 1201, and 1733.If this is intentional (e.g., parent container now provides the background), please disregard. Otherwise, consider retaining an opaque background for sticky headers.
web/app/ui/icons/Paddle.tsx (1)
1-29: LGTM!Good use of
useId()to generate unique clipPath IDs, which prevents ID collisions when multiple instances of this component are rendered on the same page.backend/migrations/clickhouse/initialise_database.js (2)
224-224: Verify PII storage compliance for customer_email.Storing
customer_emailin the analytics database may have GDPR/CCPA implications. Ensure this aligns with your data retention policies and privacy documentation, and consider whether hashing or pseudonymization would be more appropriate for analytics purposes.
209-233: Revenue table schema looks well-designed.The schema appropriately uses:
ReplacingMergeTree(synced_at)for idempotent transaction updatesDecimal64(4)for monetary amounts (4 decimal places)LowCardinalityfor low-entropy string columns (provider, type, status, currencies)- Appropriate compression codecs
web/app/pages/Project/Traffic/View/TrafficView.tsx (2)
261-316: Well-structured chart metrics with proper conflict handling.The revenue metric is correctly gated for non-selfhosted deployments and properly defines conflicts with bounce rate and session duration metrics. The
.filter(Boolean)pattern cleanly handles the conditional inclusion.
764-780: Correct dependency addition for revenue metric toggle.Adding
activeChartMetrics.revenueto the dependency array ensures analytics reload when the revenue metric is toggled, which is the expected behavior.backend/apps/cloud/src/revenue/dto/revenue-stats.dto.ts (1)
3-27: DTOs are well-structured with appropriate Swagger documentation.The remaining DTOs (
RevenueStatsDto,RevenueChartDto,RevenueBreakdownItemDto,RevenueBreakdownDto,RevenueStatusDto) are properly defined with clear property descriptions and correct typing.RevenueStatusDtocorrectly usesrequired: falsefor its optional fields.Also applies to: 29-41, 84-113, 115-129
backend/apps/cloud/src/revenue/dto/get-revenue.dto.ts (1)
44-71: Pagination and filtering DTOs are well-implemented.
GetRevenueTransactionsDtocorrectly extends the base DTO and adds pagination with sensible defaults and bounds (take: 1-100, skip: ≥0). The@Type(() => Number)decorator ensures query string parameters are properly converted.backend/apps/community/src/common/utils.ts (2)
302-316: Clean implementation of pinned-first ordering.The two-stage ordering logic correctly prioritizes pinned projects first (
isPinned DESC) while maintaining the user's selected sort preference as a secondary criterion. The approach is consistent and well-structured.
318-363: Query implementation is secure and consistent.Both search and non-search query paths:
- Use parameterized queries preventing SQL injection
- Correctly apply the LEFT JOIN for pinned status
- Maintain consistent
isPinnedcomputation via CASE expressionThe
ILIKEoperator provides case-insensitive search which is appropriate for project name/ID lookups.web/app/pages/Project/View/ViewProject.tsx (1)
78-78: LGTM - Helpful architectural documentation.The comment clearly explains that revenue data is now integrated into TrafficView, Pageflow, and Profile details instead of having a dedicated dashboard. This helps future developers understand the design decision.
web/app/api/index.ts (3)
2435-2468: LGTM - Well-structured Revenue API types.The Revenue API type definitions are clear and comprehensive. The interfaces for
RevenueStatus,RevenueStats, andRevenueChartprovide good type safety for the frontend.
2495-2524: LGTM - Clean API implementation with backward compatibility.The revenue management endpoints follow the existing patterns well:
- Consistent error handling
- Proper TypeScript return types
- Backward compatibility aliases (
connectPaddleRevenue,disconnectPaddleRevenue) ensure existing code won't break
2525-2541: LGTM - Flexible revenue data retrieval.The
getRevenueDataendpoint properly supports optional parameters for period, date ranges, timezone, and time bucket, making it flexible for various frontend use cases.backend/apps/cloud/src/app.module.ts (1)
36-36: LGTM - Clean module integration.RevenueModule is properly imported and registered in the top-level AppModule, making revenue functionality available throughout the cloud application.
Also applies to: 94-94
web/app/lib/models/Project.ts (2)
103-104: LGTM - Clean revenue tracking for sessions.Adding optional
revenueandrefundsfields to theSessioninterface enables per-session revenue tracking. The optional markers are appropriate since not all sessions will have revenue data.
169-170: LGTM - Profile revenue aggregation support.Adding
totalRevenueandrevenueCurrencytoProfileDetailsenables displaying total revenue per user profile. Using optional fields is correct since:
- Not all projects will have revenue integration enabled
- Historical profiles won't have revenue data
- Currency information depends on revenue provider configuration
backend/apps/cloud/src/task-manager/task-manager.module.ts (1)
18-18: RevenueModule import is necessary and properly used.TaskManagerService directly depends on RevenueService and PaddleAdapter from RevenueModule (injected in constructor at lines 328-329). The circular dependency chain (TaskManager → Revenue → Analytics) is already handled with forwardRef on both sides of the Revenue-Analytics relationship, making this import safe.
web/app/pages/Project/Sessions/View/SessionsView.tsx (1)
304-304: LGTM!The
currencyprop is correctly passed to theSessionscomponent using optional chaining onproject?.revenueCurrency, which safely handles cases where the project may be undefined. This aligns with the revenue currency propagation pattern seen in other components likeProfilesView.tsxandUserDetails.tsx.web/app/styles/ProjectViewStyle.css (1)
78-81: LGTM!Removing
!importantfrom the base.bb-barstroke-width rule allows the more specific revenue chart rules to properly cascade and override when needed. This is the correct approach for managing CSS specificity.web/app/pages/Project/Profiles/View/ProfilesView.tsx (1)
340-340: LGTM!The
currencyprop is correctly passed toUserDetailswith safe optional chaining. This aligns with the revenue currency propagation pattern established across the frontend.web/app/pages/Project/Settings/ProjectSettings.tsx (2)
353-358: LGTM!The revenue tab is properly configured:
- Correctly gated with
visible: !isSelfhostedto restrict to cloud edition- Follows the established tab configuration pattern
- Icon and translation key are appropriately set
1105-1106: LGTM!The conditional rendering for the Revenue component follows the same pattern as other tabs and correctly passes the
projectIdprop.web/app/pages/Project/View/components/UserDetails.tsx (1)
396-402: LGTM!The
currencyprop is correctly forwarded to theSessionscomponent, maintaining currency context for session-level revenue display.backend/apps/cloud/src/project/project.service.ts (3)
452-466: LGTM!The database-level pinning implementation is well-designed:
- The
CASE WHENexpression correctly computesisPinnedas 1 or 0- The
LEFT JOINappropriately handles projects that aren't pinned (they'll have NULL and evaluate to 0)- The
userIdparameter is already bound viasetParameterin thewhereclause and reused in the join condition
486-505: LGTM!Good refactor moving pin sorting to the database level:
- Primary sort by
isPinned DESCensures pinned projects always appear first- Using
addOrderByfor secondary sorts correctly preserves the primary sort order- This is more efficient than the previous client-side reordering approach
1707-1708: LGTM!The comment accurately documents that client-side reordering is no longer needed since pinning is handled at the query level. This improves maintainability by explaining the architectural decision.
backend/apps/cloud/src/revenue/revenue.module.ts (1)
16-27: LGTM!The module is well-structured with appropriate use of
forwardRefto handle circular dependencies betweenRevenueModuleandAnalyticsModule. The provider exports allow other modules likeTaskManagerModuleto consume revenue functionality as needed.backend/apps/cloud/src/revenue/dto/connect-paddle.dto.ts (1)
34-43: LGTM!The DTO correctly validates ISO 4217 currency codes with the appropriate length constraint.
web/app/pages/Project/Settings/tabs/Revenue.tsx (1)
86-117: LGTM!The component's state management and data loading logic is well-structured with appropriate use of
useCallbackfor memoization and proper cleanup of loading states in thefinallyblock.backend/apps/cloud/src/project/entity/project.entity.ts (1)
174-192: LGTM! Entity fields are well-defined.The revenue-related columns are appropriately typed:
textfor encrypted API keys (accommodating variable encrypted data length),varchar(3)for ISO 4217 currency codes, anddatetimefor the sync timestamp. All fields being nullable allows existing projects to remain unaffected.Note the comment at line 30: "In case of modifying some properties here add them to the GDPR data export email template." Verify whether encrypted API keys or
revenueLastSyncAtshould be included in GDPR data exports. Encrypted keys are likely excluded intentionally, but the sync timestamp may be relevant.backend/apps/cloud/src/revenue/dto/connect-revenue.dto.ts (1)
17-38: LGTM!The
apiKeyandcurrencyfields are properly validated. The API key format validation is appropriately deferred to provider-specific logic since Stripe and Paddle have different key formats.web/public/locales/en.json (1)
890-891: Revenue-related copy and keys look consistentThe added
sales/xSales, dashboardrevenue/refunds/revenueHint, project settingsrevenuetab (including connection/help text), and the project-levelrevenueanalytics block are all consistent with existing naming and tone, and the placeholders (e.g.{{count}},{{date}}) match established patterns.Also applies to: 914-916, 1497-1553, 1708-1729
backend/apps/cloud/src/revenue/interfaces/revenue.interface.ts (1)
8-88: Well-structured domain model for revenue tracking.The interfaces and enums provide a clean, type-safe foundation for the revenue tracking system. The separation of transaction data, stats, chart data, and breakdown is appropriate for the analytics use case.
backend/apps/cloud/src/revenue/adapters/stripe.adapter.ts (1)
81-143: Good overlap window implementation to avoid missing edge-case transactions.The 60-second overlap window is a solid approach to handle timing differences between systems and avoid missing transactions at sync boundaries.
backend/apps/cloud/src/analytics/analytics.controller.ts (1)
100-110: Clean integration of revenue service into analytics controller.The imports and dependency injection are properly structured, following the existing patterns in this controller.
Also applies to: 218-218
backend/apps/cloud/src/revenue/adapters/paddle.adapter.ts (3)
79-80: Production URL change required before merge.Same as the interface file - this sandbox URL must be changed to
api.paddle.comfor production.
103-151: Solid sync implementation with proper error propagation.Good that errors are re-thrown after logging (line 147), allowing callers to handle failures appropriately. The overlap window logic mirrors the Stripe adapter correctly.
153-211: Well-implemented pagination with proper cursor handling.The pagination logic correctly handles the
has_moreflag andnextcursor from Paddle's API response. The early termination whenhas_moreis false prevents unnecessary API calls.Also applies to: 213-269
web/app/pages/Project/View/components/Pageflow.tsx (3)
154-160: Good color differentiation for transaction types.The green/orange color scheme for sale/refund events provides clear visual distinction and follows common financial UI conventions (green = positive, orange = reversal/warning).
199-229: Clean implementation of conditional rendering for financial events.The separation of sale/refund display logic from pageview/event/error using a conditional block keeps the code readable. The negative sign handling for refunds is correct.
27-52: Well-extended type definitions for revenue events.The type extensions are clean and maintain backward compatibility with existing pageview/event/error types while adding the necessary fields for financial data.
backend/apps/cloud/src/analytics/analytics.service.ts (4)
4249-4278: LGTM - Clean extraction of revenue metadata for sale/refund events.The logic correctly extracts
amountandcurrencyfrom metadata for sale/refund page types, filters them out of the returned metadata array, and parses the amount as a float. The fallback toundefinedwhen entries are not found is appropriate.
4340-4362: LGTM - Revenue events integrated into session pageflow.The UNION ALL clause correctly joins revenue data into the session events timeline with appropriate metadata fields (amount, currency, transaction_id, status, provider). The filtering by
type IN ('sale', 'refund')ensures only relevant transaction types are included.
4589-4600: LGTM - Revenue totals CTE correctly aggregates session revenue.The CTE properly calculates net revenue (sales minus refunds) and total refunds per session. Using
abs(amount)for refunds handles negative amounts correctly.
4820-4830: Profile revenue query uses correct column name.The query correctly filters by
profile_id = {profileId:String}, which matches the column name used in theinsertTransactionmethod inrevenue.service.ts. No issues found.backend/apps/cloud/src/revenue/revenue.service.ts (2)
587-621: LGTM - Transaction insertion with proper date formatting.The
insertTransactionmethod correctly formats dates for ClickHouse and handles optional fields with null coalescing. The metadata serialization usingJSON.stringifyis appropriate.
188-292: LGTM - Revenue stats query with period comparison.The stats query correctly uses a subquery with
argMaxto get the latest state per transaction, preventing duplicate counting. The period comparison calculation is well-implemented.
|
@CodeRabbit is it good now? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 3053 🏁 Script executed: Length of output: 86 🏁 Script executed: Length of output: 2105 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 27 🏁 Script executed: Length of output: 1675 🏁 Script executed: Length of output: 2886 🏁 Script executed: Length of output: 2834 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 4782 🏁 Script executed: Length of output: 5056 🏁 Script executed: Length of output: 1178 🏁 Script executed: Length of output: 85 🏁 Script executed: Length of output: 7542 🏁 Script executed: Length of output: 4263 🏁 Script executed: Length of output: 906 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 2578 🏁 Script executed: Length of output: 4299 🏁 Script executed: Length of output: 4772 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 3003 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 120 🏁 Script executed: Length of output: 949 🏁 Script executed: Length of output: 1158 @Blaumaus: I've completed a comprehensive review of your Revenue Analytics PR. This is a substantial feature addition with solid architecture, but there are several important issues that need attention before merging: 🔴 Critical Security Issues1. Missing Rate Limiting on Public EndpointsThe new @Post('profile-id')
@Public()
async getOrCreateProfileId(...)Recommendation: Add rate limiting similar to other public endpoints: @Post('profile-id')
@Public()
async getOrCreateProfileId(
@Body() dto: GetProfileIdDto,
@Headers() headers,
@Ip() reqIP,
): Promise<{ profileId: string | null }> {
const ip = getIPFromHeaders(headers, true) || reqIP || ''
await checkRateLimit(ip, 'revenue-attribution', 100, 1800)
// ... rest of implementation
}2. Encryption Implementation ✅The encryption looks secure - using AES-256-CBC with proper IV generation and scrypt for key derivation. Good job on this!
|
…an option at metricsVis dropdown
1 participant
Changes
Community Edition support
Database migrations
Documentation
Summary by CodeRabbit
Release Notes
New Features
Enhancements
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.