feat(kit): analytics dashboard with revenue / MRR / churn (closes #129)#131
Conversation
…oses #129) Adds an Analytics tab that visualizes revenue, active subs, new subs, renewals, cancellations, refunds, and churn — split by platform (iOS/Android), product, and currency, with daily/weekly/monthly aggregation toggles over a 7/30/90-day range. Backend - New `revenueMetricsDaily` populator (`recomputeAllRevenueMetrics` picker + per-project mutation) wired to a daily cron. Reads the `webhookEvents` log over a trailing 3-day window so late ASN v2 / RTDN notifications fold into their correct day's bucket. Schema: added `platform` to `revenueMetricsDaily` + matching index so iOS/Android revenue can be charted separately. - New `getRevenueMetrics` query reads pre-computed rollups; all filters (range, platform, product, currency) are derived in JS to avoid Convex refetch flicker on filter clicks. Frontend - Analytics tab + page with 5 charts (Revenue / Active subs / New + Renewed / Cancellations + Refunds / Churn rate), platform cards (clickable filters, purchases-style gradient), product / currency dropdowns, range + period chiclet groups with primary-bordered active state. - Purchases cards: surface the active filter selection (was hover-only). Docs - New /docs/analytics section explaining the webhook prerequisite, setup checklist, rollup mechanics, currency/FX, churn definition, limitations. - In-page amber callout on the Analytics tab linking to Webhooks tab + setup guide. Tests - 48 new unit + integration tests covering pure helpers, every event type branch, multi-dimensional bucketing, window boundaries, idempotency, project isolation, and stale-row cleanup. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
hyochan
added
🧑🔬 analytics
kit
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a platform-split daily revenue rollup: schema changes, a 10-minute cron to schedule recomputes, a server-side recompute engine (events + subscriptions passes) with scheduler mutations, a query API, comprehensive tests, a React analytics page with charts/filters, docs, and a ChangesAnalytics Dashboard
Sequence Diagram(s)sequenceDiagram
participant Scheduler as Scheduler
participant Cron as Cron Job
participant Recompute as Recompute Engine
participant WebhookDB as Webhook Events Table
participant SubDB as Subscriptions Table
participant MetricsDB as revenueMetricsDaily Table
participant Frontend as Frontend (Analytics Page)
rect rgba(70, 130, 180, 0.5)
Scheduler->>Cron: Trigger (every 10 minutes)
Cron->>Recompute: recomputeAllRevenueMetrics(batchSize)
Recompute->>WebhookDB: Scan webhookEvents (window + grace)
WebhookDB-->>Recompute: Event stream
Recompute->>SubDB: Paginate subscriptions by state
SubDB-->>Recompute: Subscription docs
Recompute->>MetricsDB: Delete window rows
Recompute->>MetricsDB: Insert computed rollups
end
rect rgba(34, 139, 34, 0.5)
Frontend->>Recompute: useQuery(getRevenueMetrics)
Recompute->>MetricsDB: Query by project & day range
MetricsDB-->>Frontend: Precomputed rows
Frontend->>Frontend: Filter/aggregate, render charts & cards
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive Analytics dashboard for the OpenIAP kit, featuring daily revenue rollups, subscription lifecycle metrics, and multi-platform support. Key backend changes include a new revenueMetricsDaily table, a background cron job for metric recomputation, and a dedicated query for the dashboard. Feedback focuses on critical runtime errors regarding incorrect index usage in Convex queries, a schema mismatch for the platform field, and logic flaws in the cron scheduling and filter result extraction.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request introduces an Analytics dashboard for tracking revenue, subscription lifecycle metrics, and churn. Key additions include a daily cron job for data rollups, a new revenueMetricsDaily table, and a frontend implementation using Recharts. Feedback focuses on ensuring the cron job correctly rotates through projects, aligning the platform schema with legacy data requirements, optimizing database queries to use the most efficient indices, and adjusting document read caps to stay within Convex's mutation limits. The PR also includes UI improvements for purchase stat cards.
There was a problem hiding this comment.
Pull request overview
Adds an Analytics surface to the Kit project dashboard, backed by a new Convex rollup pipeline that aggregates webhook-driven subscription/revenue events into daily buckets for charting and filtering.
Changes:
- Introduces a new Project Analytics tab (UI + charts) with client-side range/period/platform/product/currency filtering.
- Adds backend daily revenue metrics rollup (
revenueMetricsDaily) + cron + query endpoint (getRevenueMetrics) and accompanying tests. - Extends docs/navigation with a new Analytics docs page explaining setup requirements and metric definitions.
Reviewed changes
Copilot reviewed 14 out of 16 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/kit/src/pages/docs/sections/analytics.tsx | New docs page describing analytics setup, rollup behavior, and metric definitions. |
| packages/kit/src/pages/docs/routes.tsx | Registers /docs/analytics route. |
| packages/kit/src/pages/docs/nav.ts | Adds Analytics entry to docs navigation. |
| packages/kit/src/pages/auth/organization/project/purchases.tsx | Improves filter card active-state styling + accessibility (aria-pressed). |
| packages/kit/src/pages/auth/organization/project/index.tsx | Adds “Analytics” tab to project dashboard navigation (Beta badge). |
| packages/kit/src/pages/auth/organization/project/analytics.tsx | New analytics dashboard UI (cards, selectors, charts, client-side aggregation). |
| packages/kit/src/pages/auth/index.tsx | Wires the new authenticated analytics route. |
| packages/kit/package.json | Adds recharts dependency for chart rendering. |
| packages/kit/convex/subscriptions/revenueMetrics.ts | Implements trailing-window recompute that populates revenueMetricsDaily. |
| packages/kit/convex/subscriptions/revenueMetrics.test.ts | Adds unit + in-memory DB integration tests for rollup behavior. |
| packages/kit/convex/subscriptions/query.ts | Adds getRevenueMetrics query to serve rollup rows + available filter values. |
| packages/kit/convex/schema.ts | Adds platform to revenueMetricsDaily and new related index. |
| packages/kit/convex/crons.ts | Schedules daily revenue-metrics recompute cron. |
| packages/kit/convex/_generated/api.d.ts | Updates generated API typings to include revenueMetrics module. |
| bun.lock | Locks new dependency graph including recharts and transitive deps. |
| .claude/settings.json | Updates Claude tool permissions configuration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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
🧹 Nitpick comments (7)
packages/kit/convex/subscriptions/revenueMetrics.ts (2)
71-82: 💤 Low valueUse
interfaceforRollupBucketper repo guidelines.-export type RollupBucket = { +export interface RollupBucket { day: string; productId: string; currency: string; platform: Platform; activeSubs: number; newSubs: number; renewals: number; cancellations: number; refunds: number; revenueMicros: number; -}; +}As per coding guidelines: "Prefer interface for defining object shapes in TypeScript".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/kit/convex/subscriptions/revenueMetrics.ts` around lines 71 - 82, The type alias RollupBucket should be converted to an interface per repo guidelines; replace the exported type RollupBucket = { ... } with export interface RollupBucket { day: string; productId: string; currency: string; platform: Platform; activeSubs: number; newSubs: number; renewals: number; cancellations: number; refunds: number; revenueMicros: number; } so all references to RollupBucket continue to work but follow the preferred interface style.
99-118: 💤 Low valuePicker
SCAN_CAPmay underfill on multi-currency projects.
subscriptionStatsis keyed by(projectId, currency)so a project running USD/EUR/JPY contributes 3 rows. Withlimit = 50→SCAN_CAP = max(150, 300) = 300, a deployment where the stalest projects each have ~5 currencies would see only ~60 distinct projects per scan window — fewer than the requested 50 in the worst case is unlikely, but the picker can also burn its cap on a single hot project's many-currency tail before reaching others.Not a release blocker (the cron runs daily and the rolling pick rebalances over time), but worth either:
- bumping the cap multiplier (
limit * 10), or- iterating with
.paginate()untilprojects.length === limitor the table is exhausted.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/kit/convex/subscriptions/revenueMetrics.ts` around lines 99 - 118, The SCAN_CAP calculation can undercount distinct projects because subscriptionStats rows are per (projectId, currency); update the picker in revenueMetrics.ts (the subscriptionStats query that uses withIndex("by_updated_at"), SCAN_CAP, limit and args.batchSize) to ensure you collect `limit` distinct projects: either increase the multiplier (e.g., use Math.max(limit * 10, 300)) or switch to a paginated scan (use .paginate() on the subscriptionStats index and continue fetching pages until projects.length === limit or the table is exhausted), deduplicating by seen projectId as you do now.packages/kit/src/pages/auth/organization/project/purchases.tsx (2)
348-376: ⚡ Quick winExtract the 5-way card dispatch — onClick and onKeyDown duplicate the same logic.
The if/else-if chain at lines 349-359 is repeated verbatim at lines 364-374. Any future change to filter behavior (e.g., adding a new card key) has to be made in two places, and forgetting one silently breaks keyboard parity with mouse.
♻️ Proposed refactor — single dispatch helper
+ const dispatchCard = (key: CardKey) => { + if (key === "total") { + resetFilters(); + } else if (key === "apple") { + applyStoreFilter("apple"); + } else if (key === "google") { + applyStoreFilter("google"); + } else if (key === "valid") { + applyValidityFilter(true); + } else if (key === "invalid") { + applyValidityFilter(false); + } + }; return ( <div key={stat.key} ... - onClick={() => { - if (stat.key === "total") { - resetFilters(); - } else if (stat.key === "apple") { - applyStoreFilter("apple"); - } else if (stat.key === "google") { - applyStoreFilter("google"); - } else if (stat.key === "valid") { - applyValidityFilter(true); - } else if (stat.key === "invalid") { - applyValidityFilter(false); - } - }} - onKeyDown={(event) => { - if (event.key === "Enter" || event.key === " ") { - event.preventDefault(); - if (stat.key === "total") { - resetFilters(); - } else if (stat.key === "apple") { - applyStoreFilter("apple"); - } else if (stat.key === "google") { - applyStoreFilter("google"); - } else if (stat.key === "valid") { - applyValidityFilter(true); - } else if (stat.key === "invalid") { - applyValidityFilter(false); - } - } - }} + onClick={() => dispatchCard(stat.key)} + onKeyDown={(event) => { + if (event.key === "Enter" || event.key === " ") { + event.preventDefault(); + dispatchCard(stat.key); + } + }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/kit/src/pages/auth/organization/project/purchases.tsx` around lines 348 - 376, Extract the duplicated 5-way dispatch into a single helper (e.g., handleCardActivate) that accepts stat.key (or stat) and performs the resetFilters / applyStoreFilter("apple") / applyStoreFilter("google") / applyValidityFilter(true|false) logic; then call that helper from both the onClick and the onKeyDown handlers (on Enter/Space) so the selection behavior for stat.key is implemented in one place and remains consistent between mouse and keyboard.
324-333: 💤 Low valueOptional: collapse the nested ternary for
isActiveinto a lookup.The 5-level ternary works but obscures the intent. A small
Record<CardKey, boolean>(or switch) reads at a glance and is friendlier to add a 6th card key to later.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/kit/src/pages/auth/organization/project/purchases.tsx` around lines 324 - 333, Replace the nested ternary that computes isActive with a small lookup map (e.g. a Record<CardKey, boolean>) or a switch: create a mapping where "total" maps to (!storeFilter && isValidFilter === undefined), "apple" -> (storeFilter === "apple"), "google" -> (storeFilter === "google"), "valid" -> (isValidFilter === true) and the default/other key -> (isValidFilter === false), then set isActive = map[stat.key] (or the switch result). Update the code around stat.key usage in the purchases.tsx component so future keys can be added easily.packages/kit/convex/subscriptions/query.ts (1)
500-508: 💤 Low valueDefensive cap on the rollup scan.
.collect()is unbounded. For a 90-day window on a project with many products × currencies × 2 platforms, this can grow large (no hard ceiling because the table is keyed-not-bounded). Convex enforces a 4 MiB / 16k-document mutation read cap, which a deeply-segmented project might bump into mid-render and surface as a 500 to the dashboard.A
.take(N)with a comfortable cap (say 16k rows = 90 days × ~10 products × ~3 currencies × 2 platforms × buffer) lets you fail predictably with a "too many rollups" error instead of crashing the query.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/kit/convex/subscriptions/query.ts` around lines 500 - 508, The unbounded .collect() on ctx.db.query("revenueMetricsDaily").withIndex("by_project_and_day_and_currency", ...) can return extremely large result sets; replace .collect() with .take(16000) (define a constant like MAX_ROLLUP_ROWS = 16000) and assign to rows, then check if rows.length === MAX_ROLLUP_ROWS and throw a clear, predictable error (e.g., "too many rollups / increase cap or reduce query window") so the query fails with a controlled message instead of hitting Convex's read cap; update any callers/handlers of the surrounding function to surface that error as needed.packages/kit/src/pages/auth/organization/project/analytics.tsx (2)
234-289: 💤 Low valueHoist the duplicate
cardRowsfilter outsidePLATFORM_CARDS.map.The filter at lines 241-248 is identical for all three cards (it intentionally excludes the platform predicate so each card can run its own platform breakdown via
totalsForPlatform). Computing it inside the map walksmetrics.daysthree times per render. Hoist it once above the map to keep theO(n)work and make the breakdown intent clearer.♻️ Hoist `cardRows` once
+ const platformCardRows = metrics.days.filter((row) => { + if (row.day < fromDay) return false; + if (selectedCurrency && row.currency !== selectedCurrency) return false; + if (selectedProduct && row.productId !== selectedProduct) return false; + return true; + }); + <div className="grid gap-4 md:grid-cols-3"> {PLATFORM_CARDS.map((card) => { const active = platformFilter === card.filter; - // Cards reflect the selected range / currency / product - // (everything except the platform filter — the cards ARE - // the platform breakdown). Filtering to fromDay matches - // what the charts below show. - const cardRows = metrics.days.filter((row) => { - if (row.day < fromDay) return false; - if (selectedCurrency && row.currency !== selectedCurrency) - return false; - if (selectedProduct && row.productId !== selectedProduct) - return false; - return true; - }); - const cardTotals = totalsForPlatform(cardRows, card.filter, currency); + const cardTotals = totalsForPlatform( + platformCardRows, + card.filter, + currency, + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/kit/src/pages/auth/organization/project/analytics.tsx` around lines 234 - 289, Hoist the identical filter over metrics.days out of the PLATFORM_CARDS.map to avoid iterating three times: compute cardRows once (using fromDay, selectedCurrency, selectedProduct on metrics.days) above the map, then pass that prefiltered array into the map so each card can still call totalsForPlatform(cardRows, card.filter, currency) and use PLATFORM_CARDS, platformFilter, totalsForPlatform, and cardTotals as before.
33-33: ⚡ Quick winUse
interfacefor object shapes per the repository's TypeScript guideline.
ProjectContext(line 33),DailyRow(lines 644-652), and the inline parameter type fortotalsForPlatform(lines 820-829) are object-shape declarations. The repository convention is to declare these asinterface; reservetypefor unions / literal aliases likePlatform,PlatformFilter,RangeId,PeriodId, andPlatformCardKey, which are correctly typed.♻️ Proposed change for the three object-shape aliases
-type ProjectContext = { project: Doc<"projects"> }; +interface ProjectContext { project: Doc<"projects"> }-type DailyRow = { - day: string; - activeSubs: number; - newSubs: number; - renewals: number; - cancellations: number; - refunds: number; - revenueMicros: number; -}; +interface DailyRow { + day: string; + activeSubs: number; + newSubs: number; + renewals: number; + cancellations: number; + refunds: number; + revenueMicros: number; +}For
totalsForPlatform, hoist the row shape to a named interface as well:+interface PlatformRow { + platform: Platform; + activeSubs: number; + newSubs: number; + revenueMicros: number; + day: string; +} + function totalsForPlatform( - rows: Array<{ - platform: Platform; - activeSubs: number; - newSubs: number; - revenueMicros: number; - day: string; - }>, + rows: PlatformRow[], filter: PlatformFilter, _currency: string, ): { revenueMicros: number; activeSubs: number; newSubs: number } {As per coding guidelines: "Prefer interface for defining object shapes in TypeScript".
Also applies to: 644-652, 820-829
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/kit/src/pages/auth/organization/project/analytics.tsx` at line 33, The code uses type aliases for object shapes contrary to project TS guidelines; change the object-shape declarations to interfaces: replace the type ProjectContext = { project: Doc<"projects"> } with an interface ProjectContext { project: Doc<"projects"> }, convert the DailyRow type alias to interface DailyRow { ... } (the shape currently defined at lines 644-652), and hoist the inline object type used in the totalsForPlatform parameter into a named interface (e.g., TotalsRow or similar) and use that interface as the parameter type in totalsForPlatform; keep union/literal aliases (Platform, PlatformFilter, RangeId, PeriodId, PlatformCardKey) as types.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/kit/convex/schema.ts`:
- Around line 725-731: The comment describing a nullable/empty-sentinel platform
is out of sync with the schema: the platform field in the schema (platform:
v.union(v.literal("IOS"), v.literal("Android")) in schema.ts) is required, while
the comment claims a nullable "" sentinel exists; reconcile them by either
removing or updating the comment to state platform is required and only
"IOS"|"Android", or if you intend to support legacy/nullable values then change
the schema validator to optional (e.g. platform:
v.optional(v.union(v.literal("IOS"), v.literal("Android"))) and update the
corresponding return validator in getRevenueMetrics (query.ts) and any insertion
paths such as webhookEvents to handle the optional/"" sentinel consistently.
In `@packages/kit/convex/subscriptions/query.ts`:
- Around line 510-527: The UI filter options are built from the already-filtered
rows, so selecting a filter (args.productId / args.currency / args.platform)
removes other choices; fix by computing the available sets from the unfiltered
dataset before applying the in-memory filters: e.g., capture the original rows
(originalRows or allRows) and build currencies, productIds, and platforms from
that full set, then apply the existing filtering logic to derive the filtered
rows returned to the UI; reference the variables rows, currencies, productIds,
platforms and the filter inputs args.productId, args.currency, args.platform to
locate and update the logic in query.ts.
In `@packages/kit/convex/subscriptions/revenueMetrics.ts`:
- Around line 178-209: The loop over events uses receivedAt to compute the
bucket day (utcDayKey(event.receivedAt)) which misattributes late-arriving
webhooks; instead keep the query range on receivedAt but compute the bucket day
from occurredAt (e.g., day = utcDayKey(event.occurredAt) with a safe fallback to
receivedAt if occurredAt is missing) before calling
bucketKey/getOrCreateBucket/applyEventToBucket. Also ensure commitBuckets'
delete-sweep or retention window is adjusted to cover the union of occurredAt
days written in this tick (or document/handle out-of-window updates) so rows for
older occurredAt days aren't erroneously deleted.
In `@packages/kit/src/pages/auth/organization/project/analytics.tsx`:
- Around line 142-155: The code allows rows from all currencies when
selectedCurrency is null, causing totals.revenueMicros and series to sum mixed
currencies but formatMicros uses currency = metrics.currencies[0]; fix by
coercing a single currency for filtering when none is chosen: set a local
fallbackCurrency = selectedCurrency ?? metrics.currencies[0] ?? "" and use
fallbackCurrency in the filteredRows predicate (replace checks of
selectedCurrency with fallbackCurrency), and likewise use fallbackCurrency when
formatting; apply the same coercion to the per-card filter block later that
mirrors filteredRows so those card totals also use the single-currency fallback.
---
Nitpick comments:
In `@packages/kit/convex/subscriptions/query.ts`:
- Around line 500-508: The unbounded .collect() on
ctx.db.query("revenueMetricsDaily").withIndex("by_project_and_day_and_currency",
...) can return extremely large result sets; replace .collect() with
.take(16000) (define a constant like MAX_ROLLUP_ROWS = 16000) and assign to
rows, then check if rows.length === MAX_ROLLUP_ROWS and throw a clear,
predictable error (e.g., "too many rollups / increase cap or reduce query
window") so the query fails with a controlled message instead of hitting
Convex's read cap; update any callers/handlers of the surrounding function to
surface that error as needed.
In `@packages/kit/convex/subscriptions/revenueMetrics.ts`:
- Around line 71-82: The type alias RollupBucket should be converted to an
interface per repo guidelines; replace the exported type RollupBucket = { ... }
with export interface RollupBucket { day: string; productId: string; currency:
string; platform: Platform; activeSubs: number; newSubs: number; renewals:
number; cancellations: number; refunds: number; revenueMicros: number; } so all
references to RollupBucket continue to work but follow the preferred interface
style.
- Around line 99-118: The SCAN_CAP calculation can undercount distinct projects
because subscriptionStats rows are per (projectId, currency); update the picker
in revenueMetrics.ts (the subscriptionStats query that uses
withIndex("by_updated_at"), SCAN_CAP, limit and args.batchSize) to ensure you
collect `limit` distinct projects: either increase the multiplier (e.g., use
Math.max(limit * 10, 300)) or switch to a paginated scan (use .paginate() on the
subscriptionStats index and continue fetching pages until projects.length ===
limit or the table is exhausted), deduplicating by seen projectId as you do now.
In `@packages/kit/src/pages/auth/organization/project/analytics.tsx`:
- Around line 234-289: Hoist the identical filter over metrics.days out of the
PLATFORM_CARDS.map to avoid iterating three times: compute cardRows once (using
fromDay, selectedCurrency, selectedProduct on metrics.days) above the map, then
pass that prefiltered array into the map so each card can still call
totalsForPlatform(cardRows, card.filter, currency) and use PLATFORM_CARDS,
platformFilter, totalsForPlatform, and cardTotals as before.
- Line 33: The code uses type aliases for object shapes contrary to project TS
guidelines; change the object-shape declarations to interfaces: replace the type
ProjectContext = { project: Doc<"projects"> } with an interface ProjectContext {
project: Doc<"projects"> }, convert the DailyRow type alias to interface
DailyRow { ... } (the shape currently defined at lines 644-652), and hoist the
inline object type used in the totalsForPlatform parameter into a named
interface (e.g., TotalsRow or similar) and use that interface as the parameter
type in totalsForPlatform; keep union/literal aliases (Platform, PlatformFilter,
RangeId, PeriodId, PlatformCardKey) as types.
In `@packages/kit/src/pages/auth/organization/project/purchases.tsx`:
- Around line 348-376: Extract the duplicated 5-way dispatch into a single
helper (e.g., handleCardActivate) that accepts stat.key (or stat) and performs
the resetFilters / applyStoreFilter("apple") / applyStoreFilter("google") /
applyValidityFilter(true|false) logic; then call that helper from both the
onClick and the onKeyDown handlers (on Enter/Space) so the selection behavior
for stat.key is implemented in one place and remains consistent between mouse
and keyboard.
- Around line 324-333: Replace the nested ternary that computes isActive with a
small lookup map (e.g. a Record<CardKey, boolean>) or a switch: create a mapping
where "total" maps to (!storeFilter && isValidFilter === undefined), "apple" ->
(storeFilter === "apple"), "google" -> (storeFilter === "google"), "valid" ->
(isValidFilter === true) and the default/other key -> (isValidFilter === false),
then set isActive = map[stat.key] (or the switch result). Update the code around
stat.key usage in the purchases.tsx component so future keys can be added
easily.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9f7299f7-5953-42b9-abd0-caccc245148e
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.lockpackages/kit/convex/_generated/api.d.tsis excluded by!**/_generated/**
📒 Files selected for processing (13)
packages/kit/convex/crons.tspackages/kit/convex/schema.tspackages/kit/convex/subscriptions/query.tspackages/kit/convex/subscriptions/revenueMetrics.test.tspackages/kit/convex/subscriptions/revenueMetrics.tspackages/kit/package.jsonpackages/kit/src/pages/auth/index.tsxpackages/kit/src/pages/auth/organization/project/analytics.tsxpackages/kit/src/pages/auth/organization/project/index.tsxpackages/kit/src/pages/auth/organization/project/purchases.tsxpackages/kit/src/pages/docs/nav.tspackages/kit/src/pages/docs/routes.tsxpackages/kit/src/pages/docs/sections/analytics.tsx
…tation, multi-currency UI - Bucket revenue events by `occurredAt` (store-side time) instead of `receivedAt` so retry-delayed Apple ASN v2 / Google RTDN notifications fold into their correct day. Extend the scan window backward by LATE_DELIVERY_GRACE_DAYS (7) so events that arrived in a prior tick are re-read on each recompute (otherwise the delete-then-insert in commitBuckets would silently drop them). - Lower WEBHOOK_SCAN_CAP / SUBS_SCAN_CAP from 20k to 15k each so the two scans + the per-day existing lookups inside commitBuckets fit under Convex's 40k document-read mutation budget with headroom. - Add `revenueMetricsRunStatus` table for picker rotation; bump cadence to 10 minutes (was daily). Walks `by_run` ascending and upserts `lastRunAt = now` after each per-project recompute, so rotation is independent of the subscription-stats drift cron. Bootstraps from `subscriptionStats` for new projects without a status row yet. - Drop unused `by_project_and_day_and_platform` index — nothing queries through it; platform filtering happens in-memory after the trailing-window range scan via `by_project_and_day_and_currency` (project + day-range only). - Fix `getRevenueMetrics` filter dropdowns: populate currency / product / platform sets from the UNFILTERED window so the UI keeps every available option visible regardless of which filter is active. - Multi-currency analytics page: drop `allowClear` on the currency selector and always filter rows by the resolved `currency` (not raw `selectedCurrency`). Fixes a bug where clearing the selector on a multi-currency project summed USD + EUR + JPY into a single number labeled with one currency code. - Schema: drop the bogus `""` sentinel claim from the `revenueMetricsDaily.platform` comment — the upstream `webhookEvents` / `subscriptions` schemas don't allow empty platform either, so the validator stays strict. - Update `getRevenueMetrics` docstring to match actual return shape (one row per (day, currency, productId, platform), not per (day, currency)). Tests: +3 covering occurredAt bucketing, out-of-window skip, and late-delivery grace scan. 51/51 pass on revenueMetrics.test.ts. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive Analytics dashboard for tracking revenue and subscription lifecycle metrics. It implements a backend rollup system in Convex that aggregates webhook events and subscription data into daily metrics, supported by a new cron job and dedicated schema tables. The frontend utilizes Recharts for data visualization, featuring various filters and period aggregations. Feedback focuses on improving observability by logging warnings when internal scan caps are reached to avoid silent data truncation, and enhancing code maintainability by replacing magic numbers with constants.
… copy - Log a warning when WEBHOOK_SCAN_CAP or SUBS_SCAN_CAP is hit so a truncated rollup surfaces in the Convex log stream instead of appearing as a quiet day on the chart. - Extract DAY_MS = 86_400_000 in analytics.tsx (already exists on the backend in revenueMetrics.ts). - Update stale "every 24h" copy on the dashboard now that the cron cadence is 10 minutes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive Analytics dashboard for the kit package, featuring revenue rollups, subscription lifecycle metrics, and churn tracking. It includes a new Convex cron job for daily metrics computation, a getRevenueMetrics query, a React-based dashboard using recharts, and associated documentation and tests. Feedback focuses on the activeSubs calculation logic, which currently risks undercounting by scanning the least recently updated subscriptions first. Additionally, it is recommended to capture timestamps once in useMemo blocks to prevent millisecond boundary inconsistencies and to consider more efficient deletion patterns in Convex as the data scales.
…w in analytics - Replace the unbounded `take(SUBS_SCAN_CAP)` on `by_project_and_updated` (which truncated the OLDEST 15k rows — exactly the wrong half on a project where active subs renew frequently) with a state-filtered descending walk on `by_project_and_state_and_updated`. Only `Active` / `InGracePeriod` / `InBillingRetry` rows are read, paginated via scheduler-chained mutations so each page gets its own 40k document-read budget. A 50k-active-sub project now completes across ~10 chained pages instead of silently losing 35k of them. - New `recomputeRevenueMetricsPage` handler rehydrates the bucket accumulator from scheduler args and resumes pagination from the saved state-index + updatedAt watermark. Cursor format is validator-typed so the scheduler's JSON round-trip survives. - Replace per-row sequential deletes inside `commitBuckets` with `Promise.all` over the per-day collected rows so the commit phase amortizes round trips even though Convex still serializes the underlying writes inside the transaction. - Capture `Date.now()` once inside the analytics page `useMemo` so the `today` and `from` UTC-day keys derive from the same instant (otherwise a millisecond crossing midnight UTC could narrow the fetch window by a day). Tests: 51/51 still pass on revenueMetrics.test.ts; the small fixtures complete inline because every counted state's row count sits well under SUBS_PAGE_SIZE=5000, so no scheduler chain fires. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/kit/convex/schema.ts (1)
765-770: 💤 Low valueConsider documenting the per-project uniqueness invariant for
revenueMetricsRunStatus.Convex has no unique constraint, so the picker logic in
recomputeAllRevenueMetricsis responsible for ensuring at most one row perprojectId(otherwiseby_runrotation would double-pick a project and waste budget). The table comment is helpful, but a one-line note that the row is an upsert keyed byprojectId(and that callers must useby_projectto look it up before insert) would make the invariant obvious to future contributors and prevent a regression where a parallel scheduler inserts duplicates.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/kit/convex/schema.ts` around lines 765 - 770, Add a one-line documentation comment on the revenueMetricsRunStatus table explaining that it is intended to be unique per projectId and treated as an upsert keyed by projectId; mention that callers (e.g., recomputeAllRevenueMetrics) must query the by_project index to look up and update the single row for a projectId rather than inserting, to avoid duplicate rows and double-picking via the by_run index.packages/kit/convex/subscriptions/revenueMetrics.test.ts (1)
369-372: 💤 Low value
MemQuery.filter()silently drops its callback — future test additions could pass against production bugs.The harness intentionally no-ops
.filter(cb). IfrunRecompute(or any future helper exercised through this harness) starts using.filter()to narrow a result set, the in-memory query will return all rows instead of the filtered subset, and the test will quietly pass while real Convex would behave differently. The current production code only uses.withIndex(), so this isn't an active bug, but it's a subtle trap.Either implement
filterhonestly (the rest of the harness already supports the predicates needed) or throw anot implementederror so the next caller is forced to wire it up.♻️ Possible safer no-op
- filter(_cb: unknown): MemQuery { - void _cb; - return this; - } + filter(_cb: unknown): MemQuery { + throw new Error( + "MemQuery.filter is not implemented; add it before using .filter() in production code under test.", + ); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/kit/convex/subscriptions/revenueMetrics.test.ts` around lines 369 - 372, MemQuery.filter currently swallows the callback which can mask real-world behavior; update the MemQuery.filter implementation (the MemQuery.filter method referenced in tests) to apply the provided predicate instead of no-oping: accept the callback, evaluate it against each row using the existing in-memory predicate helpers (or the same predicate logic used by other methods), store/apply the resulting filter so subsequent operations (e.g., runRecompute or any chainable query methods) see the narrowed result set, and return this for chaining; alternatively, if you prefer to force explicit wiring, throw a clear "not implemented" error from MemQuery.filter so callers must handle filtering explicitly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/kit/convex/subscriptions/query.ts`:
- Around line 503-511: The range scan on revenueMetricsDaily (the query using
withIndex("by_project_and_day_and_currency")) must be capped and its requested
day span validated: add a .take(FALLBACK_SCAN_CAP) (or .take(10_000)) before
.collect() to prevent unbounded scans, and enforce a server-side limit on
args.toDay - args.fromDay (e.g., throw or clamp if greater than
ROLLING_SCAN_CAP/10_000) similar to the metricsSummary handler; update the query
that currently ends with .collect() to .take(FALLBACK_SCAN_CAP).collect() and
add a short validation block for args.fromDay/args.toDay at the start of the
handler.
---
Nitpick comments:
In `@packages/kit/convex/schema.ts`:
- Around line 765-770: Add a one-line documentation comment on the
revenueMetricsRunStatus table explaining that it is intended to be unique per
projectId and treated as an upsert keyed by projectId; mention that callers
(e.g., recomputeAllRevenueMetrics) must query the by_project index to look up
and update the single row for a projectId rather than inserting, to avoid
duplicate rows and double-picking via the by_run index.
In `@packages/kit/convex/subscriptions/revenueMetrics.test.ts`:
- Around line 369-372: MemQuery.filter currently swallows the callback which can
mask real-world behavior; update the MemQuery.filter implementation (the
MemQuery.filter method referenced in tests) to apply the provided predicate
instead of no-oping: accept the callback, evaluate it against each row using the
existing in-memory predicate helpers (or the same predicate logic used by other
methods), store/apply the resulting filter so subsequent operations (e.g.,
runRecompute or any chainable query methods) see the narrowed result set, and
return this for chaining; alternatively, if you prefer to force explicit wiring,
throw a clear "not implemented" error from MemQuery.filter so callers must
handle filtering explicitly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fc2c1fec-a7cd-4fc2-bad3-337c1c9fea37
📒 Files selected for processing (7)
.claude/settings.jsonpackages/kit/convex/crons.tspackages/kit/convex/schema.tspackages/kit/convex/subscriptions/query.tspackages/kit/convex/subscriptions/revenueMetrics.test.tspackages/kit/convex/subscriptions/revenueMetrics.tspackages/kit/src/pages/auth/organization/project/analytics.tsx
✅ Files skipped from review due to trivial changes (2)
- .claude/settings.json
- packages/kit/src/pages/auth/organization/project/analytics.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/kit/convex/subscriptions/revenueMetrics.ts
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive Analytics dashboard for the kit package, featuring visualizations for revenue, subscription lifecycle metrics, and churn. Key additions include a new Convex rollup system that processes webhook events and subscription snapshots via a 10-minute cron job, a revenueMetricsDaily schema, and a React-based dashboard using Recharts. Review feedback highlighted issues with data continuity in the charts, specifically recommending that the activeSubs counter be initialized from historical snapshots rather than zero to avoid visual dips. Additionally, it was suggested to allow negative cancellation counts in daily buckets to ensure accurate net-zero aggregation across different time periods.
…ness nits - Cap the `revenueMetricsDaily` range scan at REVENUE_SCAN_CAP=10_000 via `.take()` (was unbounded `.collect()`) so a 92-day range across a maximalist multi-SKU project stays under Convex's 32k document- scan limit. Hitting the cap surfaces a `console.warn` so a truncated chart shows up in the log stream instead of silently rendering a partial tail. - Server-side validation on `args.fromDay` / `args.toDay`: reject inverted ranges, malformed ISO dates, and spans longer than MAX_RANGE_DAYS (92, matching `analytics.tsx` RANGES). Stops a misbehaving client from forcing an unbounded scan via `fromDay = "1970-01-01"`. - Document the per-`projectId` uniqueness invariant on `revenueMetricsRunStatus` directly on the schema definition — Convex has no unique constraint, so callers must upsert via `by_project` (the canonical pattern is `markRevenueMetricsRun`). - Make `MemQuery.filter` in the revenueMetrics test harness throw "not implemented" instead of silently no-oping, so a future `.filter(...)` added to production code can't pass green against unfiltered rows. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive Analytics dashboard for tracking revenue, MRR, and subscription lifecycle metrics. Key additions include a scalable rollup system in Convex that processes webhook events and subscription snapshots via a recurring cron job, a new frontend dashboard utilizing Recharts, and associated documentation. Review feedback highlighted a bug in the platform-specific active subscription calculation for multi-product projects and recommended allowing negative cancellation values in daily buckets to ensure accurate netting during weekly or monthly aggregations.
…multi-product activeSubs Three correctness fixes from the latest Gemini review pass on PR #131: 1. Drop the per-day `cancellations < 0` clamp in `applyEventToBucket`. A `SubscriptionCanceled` on day N and a `SubscriptionUncanceled` on day N+1 must net to zero when the dashboard sums per-day rollup rows into a weekly / monthly bucket; clamping silently lost the offset. Removed the `revenueMicros` clamp at the same time — the current event mapping never subtracts revenue, so the clamp was dead code AND would mask the same cross-period netting issue if a future event type ever did. Test that asserted the clamp now asserts the negative-bucket behaviour instead. 2. Stop filtering out rows older than `fromDay` from `filteredRows` in `analytics.tsx`. The pre-range rows are needed to seed the `activeSubs` carry-forward at the start of the chart — without them, a project with active subs but no events in the selected range visibly dipped to zero on the first chart day. 3. Initialize `lastActive` in `aggregateByDay` from the most-recent pre-`fromDay` snapshot in the byDay map (was hardcoded to 0). Combines with #2 to give the chart a continuous activeSubs line from day 1 even on event-quiet projects. 4. Fix `totalsForPlatform`: previously kept only the FIRST rollup row encountered for each (platform, lastDay) tuple, silently undercounting multi-product projects whose rollup table has one row per (day, productId, currency, platform). Now sums sibling rows that share `platform + day` so all SKUs show up in the App Store / Google Play card totals. Tests: 51/51 still pass on revenueMetrics.test.ts (the one clamp test got rewritten to expect cancellations=-1). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive Analytics dashboard for tracking revenue and subscription lifecycle metrics. Key additions include a backend rollup system powered by Convex crons, a new revenueMetricsDaily schema, and a frontend dashboard utilizing Recharts. Review feedback suggests enhancing the implementation by batching database deletes to ensure scalability, adding UI indicators for truncated data when scan caps are reached, and refining currency selection logic to handle empty states more gracefully.
…ency fallback - Add `truncated: boolean` to the `getRevenueMetrics` response set whenever the underlying scan hit `REVENUE_SCAN_CAP`. The dashboard surfaces it as an amber banner above the platform cards so an operator doesn't read a partial chart as a real revenue trough — the previous `console.warn` was server-only and invisible to anyone without Convex log access. - Resolve the currency fallback through an explicit length check instead of `metrics.currencies[0] ?? ""` so the empty-project case (`metrics.currencies` is `[]`) is obvious in code and the `EmptyState` gate stays the single owner of the "no rows" UX. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive Analytics dashboard for the kit package, enabling visualization of revenue, subscription lifecycle metrics, and churn rates using Recharts. The backend implementation includes a new daily revenue rollup system in Convex that processes webhook events and subscription snapshots via a scheduled cron job. Feedback from the review highlights critical concerns regarding the reliability of the pagination logic, which currently uses non-unique timestamps and may skip records. Additionally, there are warnings about potential scalability issues where the rollup process could exceed Convex's write limits or argument size constraints during data commitment and state serialization.
…te-budget safety
- Replace the hand-rolled `lt(updatedAt, watermark)` page boundary
in `processSubsPage` with Convex's `paginate({ numItems, cursor })`
API. Pagination cursors include a stable tiebreaker (the row
`_id`), so the prior approach silently dropped rows whenever two
subscriptions shared an `updatedAt` at the page boundary. Cursor
format on the chained-mutation args is now
`{ stateIdx: number, paginationCursor: string | null }`.
- Add `commitRevenueMetricsDay` and `commitOrSchedulePerDay`. When
the bucket accumulator exceeds `COMMIT_INLINE_BUCKET_LIMIT = 500`
(i.e. ~1000 ops at commit), the commit phase fans out one
scheduled mutation per day, each with its own 8192-writes budget.
A 200-SKU × 5-currency × 2-platform project (6000 buckets, 12000
ops) used to overflow the per-mutation write limit on a single
commit; it now lands across 3 chained mutations of ~4000 ops
each. `markRevenueMetricsRun` is OCC-safe so calling it from
every per-day commit converges to a single status row even if
the commits race.
- Test harness: add `MemQuery.paginate(...)` returning offset-based
cursors. Real Convex cursors are opaque and include `_id` as a
tiebreaker; the test fixtures stay well below the page boundary
where that matters, so an offset emulation is sufficient to
round-trip the production code path.
Tests: 51/51 still pass on revenueMetrics.test.ts.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (4)
packages/kit/convex/subscriptions/revenueMetrics.test.ts (1)
442-454: 💤 Low valueTest ordering relies on
Date.now()resolution — consider usingcounteralone.
_creationTime: Date.now() + this.counterderives a stable monotonic ordering inside one test (sincecounterincrements per insert), but it leaks the host clock into otherwise pure tests. If twoMemDbinstances are constructed within the same millisecond across tests, their_creationTimeranges can overlap, which (combined with thedesc/ascorder()impl inMemQuery) can produce surprising cross‑test interference if the harness ever gets shared. Usingthis.counteralone keeps the ordering deterministic and fully decoupled from wall‑clock.♻️ Optional
- _creationTime: Date.now() + this.counter, // ensure deterministic ordering + _creationTime: this.counter, // monotonic per-instance, deterministic🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/kit/convex/subscriptions/revenueMetrics.test.ts` around lines 442 - 454, The test DB's insert method uses Date.now() in building Row._creationTime which leaks wall-clock time; in the insert function (method named insert, constructing Row with _id and _creationTime) replace the Date.now() + this.counter expression with a purely deterministic monotonic value derived from the instance counter (e.g., use this.counter or this.counter plus a fixed constant) so _creationTime no longer depends on the host clock and ordering is deterministic across tests.packages/kit/src/pages/auth/organization/project/analytics.tsx (2)
942-954: 💤 Low value
formatMicrosswallows negative values — debit-side micros render as "0" (or " 0".trim() → "0").
if (!micros) return ...is truthy for0but ALSO short‑circuits for any falsy value. More importantly, the function never handles negative inputs: a negativerevenueMicros(e.g. produced when anUncancelnet‑negates aCancelacross rollup buckets —applyEventToBucketis intentionally permitted to go negative per the test on line 162‑171) flows throughvalue.toFixed(2)and renders as"USD -9.99", while compact mode happily emits"-9.9k"for tick labels. That part is fine, but the early!microsbranch never trips for negatives, so this is mostly a code‑smell — consider tightening toif (micros === 0)for clarity and to avoid the next person tripping onNaN/undefinedaccidentally returning the zero string.♻️ Optional tightening
- if (!micros) return compact ? "0" : `${currency} 0`.trim(); + if (micros === 0) return compact ? "0" : `${currency} 0`.trim();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/kit/src/pages/auth/organization/project/analytics.tsx` around lines 942 - 954, The formatMicros function currently uses a falsy check (if (!micros)) which hides the distinction between 0 and other falsy values and is confusing for negative amounts; change that guard to explicitly check for micros === 0 and keep the same return for zero, leaving negative micros to flow through to the existing value/calculation logic in formatMicros so negatives render correctly in both compact and full modes; update any related comments if present around the formatMicros function to note that negatives are valid and handled.
110-119: 💤 Low value
maxFromDay/toDayare frozen at mount — long-running sessions miss "today" after a UTC-midnight rollover.The
useMemoat lines 110‑119 depends onMAX_RANGE_DAYS, which is a module‑level constant (RANGES[RANGES.length - 1].days). The dep array therefore never changes, somaxFromDayandtoDayare computed exactly once at mount. A dashboard left open across 00:00 UTC keeps querying yesterday's window and the chart's "today" column never advances until the user reloads — particularly noticeable for users in late‑Asia/early‑Americas timezones where mid‑afternoon local time crosses UTC midnight.A small ticker (or recomputing on focus / visibility change) keeps the window aligned without forcing a full reload.
♻️ One option — re-derive on day change via a tick state
+ const [todayTick, setTodayTick] = useState(() => utcDayKey(Date.now())); + useEffect(() => { + const onFocus = () => setTodayTick(utcDayKey(Date.now())); + window.addEventListener("focus", onFocus); + document.addEventListener("visibilitychange", onFocus); + return () => { + window.removeEventListener("focus", onFocus); + document.removeEventListener("visibilitychange", onFocus); + }; + }, []); + const { maxFromDay, toDay } = useMemo(() => { const now = Date.now(); const today = utcDayKey(now); const from = utcDayKey(now - (MAX_RANGE_DAYS - 1) * DAY_MS); return { maxFromDay: from, toDay: today }; - }, [MAX_RANGE_DAYS]); + }, [MAX_RANGE_DAYS, todayTick]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/kit/src/pages/auth/organization/project/analytics.tsx` around lines 110 - 119, maxFromDay and toDay are computed once in the useMemo (dependent only on MAX_RANGE_DAYS) so they never update across a UTC midnight rollover; update the memo to also depend on a changing tick or visibility/focus signal so utcDayKey(now) is re-evaluated periodically. Add a small state like currentTick (updated by a setInterval every minute or on document.visibilitychange/window.focus) and include it in the useMemo deps used by the existing useMemo that returns { maxFromDay, toDay }, or alternatively move the utcDayKey(now) logic into a useEffect/derived state that runs on that tick/visibility change; reference the existing symbols MAX_RANGE_DAYS, useMemo, utcDayKey, DAY_MS, maxFromDay and toDay when making the change.packages/kit/convex/subscriptions/query.ts (1)
543-558: 💤 Low valueTruncation flag has a one-row false-positive at the cap boundary.
const truncated = allRows.length === REVENUE_SCAN_CAP;will surface the "Analytics partially loaded" banner whenever the project's window happens to contain exactlyREVENUE_SCAN_CAProllup rows, even though no rows were actually dropped. Fetching one extra row and comparing strictly against the cap removes the ambiguity.♻️ Optional refinement
- const REVENUE_SCAN_CAP = 10_000; - const allRows = await ctx.db - .query("revenueMetricsDaily") - .withIndex("by_project_and_day_and_currency", (q) => - q - .eq("projectId", project._id) - .gte("day", args.fromDay) - .lte("day", args.toDay), - ) - .take(REVENUE_SCAN_CAP); - const truncated = allRows.length === REVENUE_SCAN_CAP; + const REVENUE_SCAN_CAP = 10_000; + const fetched = await ctx.db + .query("revenueMetricsDaily") + .withIndex("by_project_and_day_and_currency", (q) => + q + .eq("projectId", project._id) + .gte("day", args.fromDay) + .lte("day", args.toDay), + ) + .take(REVENUE_SCAN_CAP + 1); + const truncated = fetched.length > REVENUE_SCAN_CAP; + const allRows = truncated ? fetched.slice(0, REVENUE_SCAN_CAP) : fetched;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/kit/convex/subscriptions/query.ts` around lines 543 - 558, The truncation check can false-positive when the exact number of rows equals REVENUE_SCAN_CAP; change the query to request one more row than REVENUE_SCAN_CAP (use take(REVENUE_SCAN_CAP + 1) on the ctx.db.query("revenueMetricsDaily").withIndex(...)) and then set truncated = allRows.length > REVENUE_SCAN_CAP; if you need to keep the original cap for downstream logic, trim the results to REVENUE_SCAN_CAP after detecting truncation (use REVENUE_SCAN_CAP symbol, allRows array, and truncated flag).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/kit/convex/subscriptions/query.ts`:
- Around line 543-558: The truncation check can false-positive when the exact
number of rows equals REVENUE_SCAN_CAP; change the query to request one more row
than REVENUE_SCAN_CAP (use take(REVENUE_SCAN_CAP + 1) on the
ctx.db.query("revenueMetricsDaily").withIndex(...)) and then set truncated =
allRows.length > REVENUE_SCAN_CAP; if you need to keep the original cap for
downstream logic, trim the results to REVENUE_SCAN_CAP after detecting
truncation (use REVENUE_SCAN_CAP symbol, allRows array, and truncated flag).
In `@packages/kit/convex/subscriptions/revenueMetrics.test.ts`:
- Around line 442-454: The test DB's insert method uses Date.now() in building
Row._creationTime which leaks wall-clock time; in the insert function (method
named insert, constructing Row with _id and _creationTime) replace the
Date.now() + this.counter expression with a purely deterministic monotonic value
derived from the instance counter (e.g., use this.counter or this.counter plus a
fixed constant) so _creationTime no longer depends on the host clock and
ordering is deterministic across tests.
In `@packages/kit/src/pages/auth/organization/project/analytics.tsx`:
- Around line 942-954: The formatMicros function currently uses a falsy check
(if (!micros)) which hides the distinction between 0 and other falsy values and
is confusing for negative amounts; change that guard to explicitly check for
micros === 0 and keep the same return for zero, leaving negative micros to flow
through to the existing value/calculation logic in formatMicros so negatives
render correctly in both compact and full modes; update any related comments if
present around the formatMicros function to note that negatives are valid and
handled.
- Around line 110-119: maxFromDay and toDay are computed once in the useMemo
(dependent only on MAX_RANGE_DAYS) so they never update across a UTC midnight
rollover; update the memo to also depend on a changing tick or visibility/focus
signal so utcDayKey(now) is re-evaluated periodically. Add a small state like
currentTick (updated by a setInterval every minute or on
document.visibilitychange/window.focus) and include it in the useMemo deps used
by the existing useMemo that returns { maxFromDay, toDay }, or alternatively
move the utcDayKey(now) logic into a useEffect/derived state that runs on that
tick/visibility change; reference the existing symbols MAX_RANGE_DAYS, useMemo,
utcDayKey, DAY_MS, maxFromDay and toDay when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7399bdd3-7ac3-4bfd-83ec-680e9dc6b0f7
📒 Files selected for processing (5)
packages/kit/convex/schema.tspackages/kit/convex/subscriptions/query.tspackages/kit/convex/subscriptions/revenueMetrics.test.tspackages/kit/convex/subscriptions/revenueMetrics.tspackages/kit/src/pages/auth/organization/project/analytics.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/kit/convex/subscriptions/revenueMetrics.ts
- packages/kit/convex/schema.ts
There was a problem hiding this comment.
Code Review
This pull request implements a new Analytics dashboard for tracking revenue, subscription lifecycle metrics, and churn. Key additions include a backend rollup system in Convex that processes webhook events and subscription snapshots, a new scheduled cron for periodic recomputation, and a frontend dashboard built with Recharts. Feedback focuses on optimizing the event scan window to reduce document read overhead and resolving a potential data staleness issue in the dashboard caused by pinning date boundaries at component mount.
…y minute - Drop the `LATE_DELIVERY_GRACE_DAYS` backward extension on the `webhookEvents` scan window. The receivers in `webhooks/apple.ts` and `webhooks/google.ts` always set `receivedAt >= occurredAt` (Apple `signedDate` / Google `eventTimeMillis` is a store-side timestamp, `receivedAt` is the HTTP receive time), so any event whose `occurredAt` lands in the trailing window has `receivedAt` in the same range too. The 7-day extension was burning read budget on rows the `day < firstDay` filter immediately discarded. Test that exercised the old grace was reshaped to assert the realistic late-arrival shape (occurredAt=D2, receivedAt=TODAY). - Tick the analytics page's date pin every 60s so a dashboard left open across UTC midnight picks up the new day. `now` is held in `useState` and refreshed via `setInterval`, with `useMemo` keying on it; `utcDayKey(now)` only changes once per UTC day, so Convex's `useQuery` doesn't refetch every minute (its arg deep-equality keys on the stable day string). Tests: 51/51 still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive Analytics dashboard, featuring a new background rollup system that aggregates webhook events into daily revenue metrics. The implementation includes a Convex cron job, a new database schema for tracking processing status, and a React-based dashboard utilizing Recharts. Feedback focused on backend and frontend optimizations, specifically suggesting an increase to the revenue scan cap to prevent data truncation, utilizing more specific database indexes for product-level queries, and memoizing heavy data processing in the frontend to improve render performance.
…alytics derivations - Bump `REVENUE_SCAN_CAP` from 10k to 20k. The maximalist 92-day range described in the comment (30 SKUs × 3 currencies × 2 platforms ≈ 16.5k rows) was always tripping the prior cap, so the chart silently truncated for any project that big. 20k still sits well under Convex's 32k document-scan limit and gives the documented use case real headroom. - When `args.productId` is set, route the read through `by_project_and_product_and_day_and_currency` so the index range already filters down to the SKU's rows. The prior broad-index scan + in-memory filter could exhaust the cap on irrelevant rows (and therefore truncate the relevant ones) for projects with many products. - Wrap the analytics page's filter / aggregate / bucket / totals pipeline and the platform-card totals in `useMemo`. Without it, the per-minute `now` tick was re-running the full pipeline (and three independent filter-then-totals walks per render inside the `PLATFORM_CARDS` map) every minute. The memo deps target the filters that actually drive the data, so a UTC-day-flip recomputes once and idle minutes stay free. - Hooks-rules-of-hooks compliance: hoisted the `metrics === undefined` early return below every hook call. Hooks consume `metricsDays` / `metricsCurrencies` defaults that fall back to shared empty constants on the loading render so deps stay identity-stable. Tests: 51/51 still pass on revenueMetrics.test.ts. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive Analytics dashboard for tracking revenue, MRR, and subscription churn. Key additions include a backend rollup system that processes webhook events and subscription snapshots via a recurring cron, a new Convex query for retrieving these metrics, and a frontend visualization page using Recharts. Feedback focuses on the getRevenueMetrics query, specifically addressing a logic inconsistency where filter options are populated from a pre-filtered result set and the removal of a redundant in-memory filter for product IDs.
The dashboard does all currency / product / platform filtering client-side (the unfiltered fetch is what backs the filter-dropdown population), and `getRevenueMetrics` has no other caller. The optional `productId` / `currency` / `platform` args added in the prior round were never actually exercised — and the productId narrowed-index branch I added with them was incompatible with the dropdown contract (when productId was pinned, the dropdown silently collapsed to that SKU's currencies / platforms only). Removed: - All three optional args from the schema and the in-memory filter fall-through that consumed them. - The `by_project_and_product_and_day_and_currency` branch in the range scan; the broad `by_project_and_day_and_currency` index now always backs the read. Index itself stays defined on the table for future per-product surfaces. - The post-scan `if (args.productId) rows = rows.filter(...)` chain (now fully redundant — `allRows` IS the returned `days`). A future caller that actually needs server-side narrowing should get its own dedicated query rather than reintroducing optional args on this one (documented inline). Tests: 51/51 still pass on revenueMetrics.test.ts. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive Analytics dashboard to the kit package, featuring a new automated revenue rollup system. Key additions include a 10-minute cron job for processing webhook events and subscription snapshots, a new Convex schema for daily metrics, and a frontend dashboard implemented with Recharts. Documentation and extensive tests for the rollup logic are also included. Feedback focuses on potential scaling bottlenecks, specifically the lack of pagination in the initial webhook event scan and potential write limit issues during the daily commit phase for projects with high cardinality in product and currency combinations.
…bucket days Two scaling fixes from the latest Gemini review pass on PR #131: - The events pass now paginates via the same scheduler-chained pattern as the subs pass. `runRecompute` kicks off a phased pipeline: first an `events` phase that walks `webhookEvents` via Convex's `paginate({ numItems, cursor })` API, transitioning to the existing `subs` phase once exhausted. The chained-page cursor is now a tagged union (`{phase: "events", paginationCursor}` | `{phase: "subs", stateIdx, paginationCursor}`) so a single scheduler handler can resume from either phase. A noisy project that emits 50k events in the trailing 3-day window now paginates across ~10 chained event mutations instead of silently truncating at the prior 15k cap. - `commitRevenueMetricsDay` now checks whether `existing + nonZero` would exceed COMMIT_DAY_WRITES_LIMIT (7000, with headroom under Convex's 8192-writes-per-mutation budget). Below the limit it commits inline as before; above it, it deletes inline and fans inserts out across chained `commitRevenueMetricsDayInsertChunk` mutations of size INSERT_CHUNK_SIZE=3500 each. A project with ~10k buckets/day now lands across ~3 chained chunks instead of blowing the cap. `markRevenueMetricsRun` stays OCC-safe across the parallel chunks and the kickoff so the picker rotates regardless of chunk-completion order. Side effect: removed the `WEBHOOK_SCAN_CAP` constant (the events pass is no longer cap-bounded) and its associated truncation warning. Truncation isn't a meaningful state anymore — the events pass simply continues until done. Tests: 51/51 still pass on revenueMetrics.test.ts. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive Analytics dashboard to the kit package, featuring a new backend rollup system in Convex that processes webhook events and subscription snapshots to generate daily metrics such as revenue, active subscriptions, and churn. The frontend implementation utilizes Recharts for data visualization and includes filtering capabilities by platform, product, and currency. Feedback from the review highlights critical issues in the rollup logic concerning historical accuracy for expired subscriptions within the trailing window and potential bugs in the isActiveAt check. Additionally, a logic error was identified in the UI where platform cards and non-financial charts are incorrectly filtered by currency, leading to misleading totals on multi-currency projects.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
packages/kit/src/pages/auth/organization/project/analytics.tsx (2)
494-496: 💤 Low valueEmpty-state gate uses unfiltered
metrics.days— filtered-to-empty case still renders charts.
metrics.days.length === 0is the unfiltered server response, so when a user picks aselectedProduct/selectedCurrency/platformFiltercombination that has zero matching rows,seriesbecomes an array of N synthetic zero-rows (fromaggregateByDay'srangeDaysloop) and the dashboard renders flat-zero charts instead of theEmptyStatepanel. Consider gating onseries.length === 0 || series.every(s => isAllZero(s))so the no-match-for-filters case surfaces the same "no data" affordance.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/kit/src/pages/auth/organization/project/analytics.tsx` around lines 494 - 496, The EmptyState check currently gates on the unfiltered server response (metrics.days) so filter/no-match cases still show flat charts; update the conditional that renders EmptyState to use the derived/filtered series instead (the array produced by aggregateByDay) — e.g., replace the check using metrics.days with one that checks series.length === 0 || series.every(s => /* all values zero */) (you can add a small helper isAllZero(seriesItem) if helpful) and ensure the EmptyState renders when there are no non-zero data points; reference the variables series, aggregateByDay, metrics.days, and EmptyState to locate and change the conditional.
786-1028: ⚡ Quick winAdd unit tests for the pure helpers (
aggregateByDay,bucketByPeriod,bucketLabelFor,totalsForPlatform,formatMicros).These functions encode non-trivial business logic — carry-forward seeding for
activeSubs, ISO-week bucketing, last-day-per-platformactiveSubssumming across(product, currency)tuples, and divide-by-zero churn handling — but live as un-exported page-local helpers with no test coverage. Extracting them into a sibling module (e.g.analytics.helpers.ts) and adding a Vitest suite would protect against the same class of subtle regressions thatrevenueMetrics.test.tsalready guards on the server side.As per coding guidelines, "Write unit tests for all utility functions and business logic".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/kit/src/pages/auth/organization/project/analytics.tsx` around lines 786 - 1028, Extract the page-local helpers aggregateByDay, bucketByPeriod, bucketLabelFor, totalsForPlatform, utcDayKey, and formatMicros into a new sibling module (e.g. analytics.helpers.ts), export them, and update the original analytics.tsx to import those functions; then add a Vitest suite that unit-tests: aggregateByDay's carry-forward seeding and pre-range seed selection, bucketLabelFor ISO-week (Mon start) and monthly keys, bucketByPeriod aggregation rules and churnPct/divide-by-zero behavior, totalsForPlatform's last-day-per-platform logic and summing across same-day different (product,currency) rows, and formatMicros formatting/compact/zero cases so the pure business logic is covered.packages/kit/convex/subscriptions/revenueMetrics.test.ts (1)
546-1026: ⚖️ Poor tradeoffCoverage gap: scheduler-chained pagination + per-day commit fan-out paths are untested.
Every integration test runs with bucket / event / subscription counts well below
EVENTS_PAGE_SIZE = 5_000,SUBS_PAGE_SIZE = 5_000, andCOMMIT_INLINE_BUCKET_LIMIT = 500, so:
recomputeRevenueMetricsPage(the chained-page entry point that re-hydratesbucketsfrom scheduler args)commitRevenueMetricsDay/commitRevenueMetricsDayInsertChunk(the per-day fan-out commit path that kicks in pastCOMMIT_INLINE_BUCKET_LIMIT)- the
pageRemaining <= 0continuation branch inprocessSubsPagenever execute under the harness. The serialized
bucketsround-trip in particular is exactly the kind of code path that's easy to break with an off-by-one when a future refactor renames a field onRollupBucket. Consider lowering the page-size thresholds in the test harness via injectable constants (or seeding > 500 buckets for one targeted test) so these branches get at least smoke coverage.As per coding guidelines, "Write unit tests for all utility functions and business logic".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/kit/convex/subscriptions/revenueMetrics.test.ts` around lines 546 - 1026, The tests never exercise chained pagination and per-day fan-out branches (recomputeRevenueMetricsPage, processSubsPage continuation when pageRemaining <= 0, commitRevenueMetricsDay / commitRevenueMetricsDayInsertChunk and the RollupBucket serialization round-trip) because EVENTS_PAGE_SIZE, SUBS_PAGE_SIZE and COMMIT_INLINE_BUCKET_LIMIT are too large for the harness; add a focused integration test that either (A) makes those constants injectable via the test harness/context (e.g. add overrides to makeCtx or test config) and sets them to small values, then seed enough webhookEvents/subscriptions/buckets to force pagination and >COMMIT_INLINE_BUCKET_LIMIT per-day buckets, or (B) seed a single day's >COMMIT_INLINE_BUCKET_LIMIT RollupBucket entries and >EVENTS_PAGE_SIZE events to force recomputeRevenueMetricsPage paging and commitRevenueMetricsDayInsertChunk path; call recomputeRevenueMetricsPage directly with a serialized buckets arg to validate the buckets round-trip and assert the expected rows are written and no off-by-one behavior occurs.packages/kit/convex/subscriptions/revenueMetrics.ts (2)
123-171: 🏗️ Heavy liftNo in-flight guard against overlapping recomputes for the same project across cron ticks.
recomputeAllRevenueMetricsselects projects byrevenueMetricsRunStatus.by_runASC, butlastRunAtis only stamped at commit time insidemarkRevenueMetricsRun. Between a tick scheduling a project and its commit phase running (which can include several chainedrecomputeRevenueMetricsPagemutations + scheduler hops for noisy projects), the next 10-minute tick can re-select the same project if it's still the oldest entry (or if there are < 100 stale projects total).Two concurrent recompute chains for the same project then both delete-then-insert into
revenueMetricsDailyfor the trailing window. Convex OCC will retry conflicting commits, but if the two chains were built from slightly different event snapshots (a webhook landed between kickoffs), the loser's writes silently overwrite the winner's. For a 3-day trailing window the next tick converges, so the data isn't permanently wrong — but the row counts will flap, which can confuse anyone watching the dashboard live.A small "kickoff stamp" — patch
revenueMetricsRunStatus.lastRunAt = nowat the start ofrecomputeRevenueMetricsForProject(or use a separaterunningSincefield) — would prevent same-project double-scheduling on small deployments without changing the commit-time semantics.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/kit/convex/subscriptions/revenueMetrics.ts` around lines 123 - 171, recomputeAllRevenueMetrics can schedule the same project twice because lastRunAt is only set at commit; to fix, add a kickoff-stamp at the start of the recompute chain: in recomputeRevenueMetricsForProject (or create a new runningSince field on revenueMetricsRunStatus) write/update revenueMetricsRunStatus for the project immediately (e.g. set runningSince = now or set lastRunAt = now before doing heavy work) so subsequent recomputeAllRevenueMetrics scans will skip projects already stamped; use the existing revenueMetricsRunStatus record (and the markRevenueMetricsRun helper if appropriate) to perform the immediate write via ctx.db (insert or update) at the start of recomputeRevenueMetricsForProject to prevent overlapping runs.
411-423: ⚖️ Poor tradeoffScheduler-args payload grows with bucket count during chained event pagination — monitor for long-tail projects.
buckets: Array.from(buckets.values())ships the full per-project accumulator (every distinct(day, productId, currency, platform)tuple) into the scheduler args on every continuation page. For the realistic SaaS shape called out elsewhere in the file (≤180 tuples × 3 days ≈ 540 buckets), the payload is tiny. But a long-tail project with hundreds of SKUs and many currencies in 3 days could push tens of thousands of buckets through every continuation, which scales linearly witheventCount / EVENTS_PAGE_SIZEchained pages.Convex's per-scheduled-function-argument limit is 4 MiB; you're likely well under that today, but the scaling shape is "args size × number of chained pages" rather than bounded. On noisy projects, this becomes the dominant cost (JSON serialize/parse every page). Consider a checkpoint table keyed by
(projectId, runStartedAt)to store the in-flight accumulator, decoupling page count from arg size and making the chain restartable on failure — useful if this becomes a scaling bottleneck.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/kit/convex/subscriptions/revenueMetrics.ts` around lines 411 - 423, The scheduler chaining currently passes the full accumulator via buckets: Array.from(buckets.values()) into internal.subscriptions.revenueMetrics.recomputeRevenueMetricsPage, which will grow with bucket count and multiply by the number of continuation pages; instead persist the in-flight accumulator to a checkpoint store keyed by (projectId, runStartedAt) (e.g., a simple table keyed by projectId and runStartedAt) and change the scheduler.runAfter call to send only a small checkpoint reference (projectId + runStartedAt + optionally a checkpointId), then update recomputeRevenueMetricsPage to load the accumulator from that checkpoint, update it between pages, and write it back so args stay small and the chain can be restarted on failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/kit/src/pages/auth/organization/project/analytics.tsx`:
- Around line 168-172: The inline args object passed to
useQuery(api.subscriptions.query.getRevenueMetrics, { apiKey: project.apiKey,
fromDay: maxFromDay, toDay }) is recreated every render causing unnecessary
resubscriptions; wrap the args in a useMemo that returns { apiKey:
project.apiKey, fromDay: maxFromDay, toDay } and depend on [project.apiKey,
maxFromDay, toDay], then pass that memoized args to useQuery, and after changing
this, smoke-test the UTC midnight boundary (utcDayKey(now) roll-over) to confirm
refetch behavior is correct.
---
Nitpick comments:
In `@packages/kit/convex/subscriptions/revenueMetrics.test.ts`:
- Around line 546-1026: The tests never exercise chained pagination and per-day
fan-out branches (recomputeRevenueMetricsPage, processSubsPage continuation when
pageRemaining <= 0, commitRevenueMetricsDay / commitRevenueMetricsDayInsertChunk
and the RollupBucket serialization round-trip) because EVENTS_PAGE_SIZE,
SUBS_PAGE_SIZE and COMMIT_INLINE_BUCKET_LIMIT are too large for the harness; add
a focused integration test that either (A) makes those constants injectable via
the test harness/context (e.g. add overrides to makeCtx or test config) and sets
them to small values, then seed enough webhookEvents/subscriptions/buckets to
force pagination and >COMMIT_INLINE_BUCKET_LIMIT per-day buckets, or (B) seed a
single day's >COMMIT_INLINE_BUCKET_LIMIT RollupBucket entries and
>EVENTS_PAGE_SIZE events to force recomputeRevenueMetricsPage paging and
commitRevenueMetricsDayInsertChunk path; call recomputeRevenueMetricsPage
directly with a serialized buckets arg to validate the buckets round-trip and
assert the expected rows are written and no off-by-one behavior occurs.
In `@packages/kit/convex/subscriptions/revenueMetrics.ts`:
- Around line 123-171: recomputeAllRevenueMetrics can schedule the same project
twice because lastRunAt is only set at commit; to fix, add a kickoff-stamp at
the start of the recompute chain: in recomputeRevenueMetricsForProject (or
create a new runningSince field on revenueMetricsRunStatus) write/update
revenueMetricsRunStatus for the project immediately (e.g. set runningSince = now
or set lastRunAt = now before doing heavy work) so subsequent
recomputeAllRevenueMetrics scans will skip projects already stamped; use the
existing revenueMetricsRunStatus record (and the markRevenueMetricsRun helper if
appropriate) to perform the immediate write via ctx.db (insert or update) at the
start of recomputeRevenueMetricsForProject to prevent overlapping runs.
- Around line 411-423: The scheduler chaining currently passes the full
accumulator via buckets: Array.from(buckets.values()) into
internal.subscriptions.revenueMetrics.recomputeRevenueMetricsPage, which will
grow with bucket count and multiply by the number of continuation pages; instead
persist the in-flight accumulator to a checkpoint store keyed by (projectId,
runStartedAt) (e.g., a simple table keyed by projectId and runStartedAt) and
change the scheduler.runAfter call to send only a small checkpoint reference
(projectId + runStartedAt + optionally a checkpointId), then update
recomputeRevenueMetricsPage to load the accumulator from that checkpoint, update
it between pages, and write it back so args stay small and the chain can be
restarted on failure.
In `@packages/kit/src/pages/auth/organization/project/analytics.tsx`:
- Around line 494-496: The EmptyState check currently gates on the unfiltered
server response (metrics.days) so filter/no-match cases still show flat charts;
update the conditional that renders EmptyState to use the derived/filtered
series instead (the array produced by aggregateByDay) — e.g., replace the check
using metrics.days with one that checks series.length === 0 || series.every(s =>
/* all values zero */) (you can add a small helper isAllZero(seriesItem) if
helpful) and ensure the EmptyState renders when there are no non-zero data
points; reference the variables series, aggregateByDay, metrics.days, and
EmptyState to locate and change the conditional.
- Around line 786-1028: Extract the page-local helpers aggregateByDay,
bucketByPeriod, bucketLabelFor, totalsForPlatform, utcDayKey, and formatMicros
into a new sibling module (e.g. analytics.helpers.ts), export them, and update
the original analytics.tsx to import those functions; then add a Vitest suite
that unit-tests: aggregateByDay's carry-forward seeding and pre-range seed
selection, bucketLabelFor ISO-week (Mon start) and monthly keys, bucketByPeriod
aggregation rules and churnPct/divide-by-zero behavior, totalsForPlatform's
last-day-per-platform logic and summing across same-day different
(product,currency) rows, and formatMicros formatting/compact/zero cases so the
pure business logic is covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 001d1726-4216-4653-8439-e790f16ab3d6
📒 Files selected for processing (4)
packages/kit/convex/subscriptions/query.tspackages/kit/convex/subscriptions/revenueMetrics.test.tspackages/kit/convex/subscriptions/revenueMetrics.tspackages/kit/src/pages/auth/organization/project/analytics.tsx
…nue, memo useQuery args Three fixes from the round-12 reviewer pass on PR #131: 1. Historical activeSubs accuracy. `COUNTED_STATES` / `COUNTED_STATES_ORDERED` now include `Expired`. A sub that was active two days ago and has since expired must still contribute to the activeSubs snapshot for the day it was actually active — the prior code skipped it because the state walk filtered to non-expired counted states. `isActiveAt` is the gatekeeper now: `expiresAt > dayEnd` decides whether a counted-state sub is active on a given day, so an Expired sub with `expiresAt` past the snapshot day correctly counts, and an Expired sub with no `expiresAt` defensively counts as inactive (no transition day to anchor on). 4 new isActiveAt tests + 2 new round-trip tests. 2. Multi-currency dashboard split. The platform cards and non-revenue charts (Active subs / New + renewed / Cancellations + refunds / Churn) used to filter by the selected currency, under-reporting "All platforms" totals on multi-currency projects. Lifecycle counters (activeSubs / newSubs / renewals / cancellations / refunds) now flow through a `lifecycleRows` pipeline that is currency-UNFILTERED — counts are currency-agnostic by definition, so summing across currencies is the correct project-wide story. Revenue stays pinned to the selected currency (FX-incompatible across currencies). The final chart `series` joins lifecycle (currency-unfiltered) + revenue (currency-filtered) per bucket. Same split applies to `platformTotals` — added `lifecycleForPlatform` and `revenueForPlatform` helpers in place of the prior `totalsForPlatform`. 3. `useQuery` args memoisation. `convex/react`'s `useQuery` does NOT deep-compare args between renders — an inline `{ ... }` literal triggers a fresh subscription every render. Wrapped the args in a `useMemo` keyed on `[project.apiKey, maxFromDay, toDay]` so the underlying `convex.watchQuery` only resubscribes when an actual input changes (the per-minute `now` tick stays off the dep list because `maxFromDay` / `toDay` are derived from it via `utcDayKey(...)` and only change at UTC midnight). Tests: 55/55 (was 51, +2 new isActiveAt tests, +2 new round-trip tests for Expired inclusion). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
/gemini review |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive Analytics dashboard, featuring a background rollup engine that processes webhook events and subscription states into daily metrics. It includes new database tables, cron jobs for periodic recomputation, and a frontend interface with charts for tracking revenue, active subscriptions, and churn. Feedback was provided regarding the use of non-deterministic Date.now() within a Convex mutation, which could lead to inconsistencies during retries.
| args: { projectId: v.id("projects") }, | ||
| returns: v.null(), | ||
| handler: async (ctx, args) => { | ||
| await runRecompute(ctx, args.projectId, Date.now()); |
There was a problem hiding this comment.
Using Date.now() in a mutation is non-deterministic and can lead to inconsistencies if the mutation retries. In Convex, it is safer to pass the time from the cron job or use a deterministic approach.
Summary
revenueMetricsDailypopulator (cron + per-project mutation) readswebhookEventsover a trailing 3-day window so late ASN v2 / RTDN notifications fold into their correct day. Schema gains aplatformfield + matching index for iOS/Android revenue split. NewgetRevenueMetricsquery; all dashboard filters derive client-side to avoid Convex refetch flicker on filter clicks.Test plan
bun run lint(tsc + eslint)bun run test— 394 tests pass (48 new forrevenueMetrics)bun run format:checkbun run smoke:server(compile + /health probe)setupStatus/docs/analyticsrenders and cross-links to Webhooks tab + verification pages🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Tests
Chores