feat(kit): add reporting currency setting#136
Conversation
Add a project-level reporting currency and keep MRR/revenue totals pinned to that currency unless explicit FX conversion exists. Non-reporting currencies stay visible as excluded per-currency breakdowns.\n\nCloses #135
hyochan
added
🧑🔬 analytics
kit
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ 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 (5)
📝 WalkthroughWalkthroughAdds project-level reporting currency configuration with USD default. Backend normalizes MRR/revenue to the configured currency while tracking excluded currencies. UI allows currency selection in settings and distinguishes headline revenue (reporting currency) from chart visualization (user-selected currency) with explanatory callouts. ChangesConfigurable Reporting Currency Feature
Sequence DiagramssequenceDiagram
participant User
participant SettingsUI as Settings Form
participant API as updateProject
participant Query as metricsSummary Query
participant UI as Analytics/MRR Display
User->>SettingsUI: enter "EUR" as reporting currency
SettingsUI->>SettingsUI: validate pattern, trim/uppercase
User->>SettingsUI: click "Save currency"
SettingsUI->>API: updateProject({reportingCurrency: "EUR"})
API->>API: normalizeReportingCurrency("EUR")
API->>Query: fetch metrics with updated project
Query->>Query: selectReportingMrr(mrrByCurrency, "EUR")
Query-->>UI: {reportingCurrency: "EUR", mrrMicros: <EUR-value>, excludedMrrByCurrency: [{currency: "USD", ...}]}
UI->>UI: formatMicros(mrrMicros, {currency: "EUR"})
UI-->>User: MRR (EUR) €X.XX + excluded currencies list
sequenceDiagram
participant ChartCurrency as User-Selected<br/>Chart Currency
participant ReportingCurrency as Reporting<br/>Currency (USD)
participant Analytics as Analytics<br/>Dashboard
ChartCurrency->>Analytics: revenueRows filtered to selected currency
ReportingCurrency->>Analytics: reportingRevenueRows filtered to USD
Analytics->>Analytics: merge revenueSeries from chart currency
Analytics->>Analytics: merge totals from reportingRevenueSeries
Analytics-->>ChartCurrency: chart shows selected currency revenue
Analytics-->>ReportingCurrency: headline totals show USD revenue
Analytics-->>User: callout explains USD is pinned, other currencies excluded
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 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 per-project 'reporting currency' setting to improve MRR and revenue analytics, ensuring that totals are calculated for a specific currency rather than summing across different currencies without FX conversion. The changes include schema updates, new validation logic, UI components for configuration and display, and updated documentation. The reviewer suggested consolidating redundant 'formatMicros' utility functions across the codebase to improve maintainability and address a minor formatting bug.
There was a problem hiding this comment.
Pull request overview
Adds a project-level “reporting currency” to stabilize how Kit presents MRR and revenue analytics in multi-currency projects, avoiding cross-currency summing and making excluded currencies explicit.
Changes:
- Introduces
projects.reportingCurrency(schema + create/update mutations) and normalizes/validates it on write. - Updates subscription metrics aggregation to compute headline MRR strictly in the reporting currency and return excluded per-currency MRR.
- Updates Kit UI + docs to surface the reporting currency setting and communicate excluded-currency behavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/kit/src/pages/docs/sections/analytics.tsx | Updates analytics docs to describe reporting currency behavior and lack of FX conversion. |
| packages/kit/src/pages/auth/organization/project/subscriptions.tsx | Displays MRR labeled by reporting currency and shows an excluded-currency breakdown. |
| packages/kit/src/pages/auth/organization/project/settings.tsx | Adds a Project Settings form to set/report the reporting currency (ISO 4217). |
| packages/kit/src/pages/auth/organization/project/analytics.tsx | Defaults analytics currency selection to reporting currency and shows excluded-currency messaging. |
| packages/kit/convex/subscriptions/query.ts | Computes headline MRR in reporting currency; returns excluded per-currency MRR. |
| packages/kit/convex/subscriptions/query.test.ts | Adds unit tests for reporting-currency MRR selection logic. |
| packages/kit/convex/schema.ts | Adds optional reportingCurrency to the projects table and updates currency-related commentary. |
| packages/kit/convex/projects/mutation.ts | Sets default reporting currency on project creation and validates it on update. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a reporting currency setting for projects to ensure MRR and revenue totals are pinned to a specific currency, avoiding automatic FX conversion. Changes span the schema, backend validation, and UI, including new utility functions for currency formatting. Reviewers recommended removing redundant normalization logic in the subscription query and centralizing duplicated currency constants into a shared utility file to improve maintainability.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a 'reporting currency' feature, allowing projects to specify a primary currency for dashboard analytics. Key changes include schema updates to the projects table, new validation and normalization utilities for currency codes, and UI enhancements in the analytics, subscriptions, and settings pages to handle and display reporting-specific totals. The feedback suggests extracting the currency validation regex into a shared utility function within the Convex backend to reduce code duplication.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a project-level reporting currency setting to ensure consistent MRR and revenue displays in the dashboard. It moves currency normalization and formatting logic into shared utilities, adds a new configuration UI in the project settings, and updates the analytics and subscription views to explicitly separate reporting currency totals from other currency slices. Feedback includes a suggestion to improve rounding in compact currency formatting and a recommendation to avoid positional indexing when joining metrics series to ensure data integrity.
|
/gemini review |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Code Review
This pull request introduces a reporting currency configuration for projects, allowing users to specify a primary currency for MRR and revenue reporting. The changes include backend updates to handle currency normalization and filtering, new UI components in project settings for configuration, and updates to the analytics and subscription dashboards to display metrics pinned to the project's reporting currency. Documentation has also been updated to reflect these changes. I have no feedback to provide as there were no review comments.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
packages/kit/src/lib/utils.ts (1)
23-34: ⚡ Quick winExtract the
formatMicrosoptions to a namedinterfaceper coding guidelines.The inline anonymous object type for the options parameter doesn't comply with the project-wide rule to prefer named interfaces for object shapes.
♻️ Proposed refactor
+export interface FormatMicrosOptions { + currency?: string | null; + compact?: boolean; + emptyWhenZero?: boolean; +} + export function formatMicros( micros: number, - { - currency, - compact = false, - emptyWhenZero = false, - }: { - currency?: string | null; - compact?: boolean; - emptyWhenZero?: boolean; - } = {}, + { + currency, + compact = false, + emptyWhenZero = false, + }: FormatMicrosOptions = {}, ): string {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/src/lib/utils.ts` around lines 23 - 34, The options object type for formatMicros is currently an inline anonymous type; extract it to a named exported interface (e.g., FormatMicrosOptions) and replace the inline type in the formatMicros signature with that interface, preserving the same optional properties (currency?: string | null, compact?: boolean, emptyWhenZero?: boolean) and default parameter value handling; update any imports/exports so the new interface is exported if used externally and ensure callers continue to work without code changes.packages/kit/src/lib/utils.test.ts (1)
5-13: ⚡ Quick winAdd coverage for
nullandundefinedinputs tonormalizeCurrencyCode.The function signature explicitly accepts
string | null | undefined, but neither branch is exercised by the current tests. The fallback-without-explicit-argument path (relying onDEFAULT_REPORTING_CURRENCY) is also untested.✅ Suggested additional test cases
it("falls back when the currency code is invalid", () => { expect(normalizeCurrencyCode("usdollar", "EUR")).toBe("EUR"); }); + + it("falls back to DEFAULT_REPORTING_CURRENCY when input is null", () => { + expect(normalizeCurrencyCode(null)).toBe("USD"); + }); + + it("falls back to DEFAULT_REPORTING_CURRENCY when input is undefined", () => { + expect(normalizeCurrencyCode(undefined)).toBe("USD"); + }); });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/lib/utils.test.ts` around lines 5 - 13, Add unit tests for normalizeCurrencyCode to cover null and undefined inputs and the default-fallback path: write tests that call normalizeCurrencyCode(null) and normalizeCurrencyCode(undefined) asserting they return the provided fallback or DEFAULT_REPORTING_CURRENCY when no fallback arg is passed, and add a test that calls normalizeCurrencyCode(undefined, "GBP") to verify explicit fallback behavior; reference the normalizeCurrencyCode function and DEFAULT_REPORTING_CURRENCY constant in your new test cases.packages/kit/src/pages/auth/organization/project/analytics.tsx (2)
570-579: 💤 Low valueMinor: callout copy can be confusing when the chart currency equals the reporting currency.
When
currency === reportingCurrency, the callout reads roughly: "Headline revenue totals are pinned to USD. EUR, JPY are excluded from headline totals…". That reads fine. But when the user picks a non-reporting currency (e.g. EUR) for the chart, EUR is the chart currency yet still appears inexcludedReportingCurrencies— so the message says "The revenue chart is showing EUR. EUR, JPY are excluded from headline totals…", which is technically accurate but feels self-contradictory at a glance.Consider listing only currencies other than both
reportingCurrencyand the active chartcurrencyin the excluded label, or rewording to "headline totals only include {reportingCurrency}; EUR and JPY are not converted" so the chart currency call-out and the excluded list don't appear to fight.🤖 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 570 - 579, The callout should not list the active chart currency as an "excluded" currency; update the JSX block that references excludedReportingCurrencies/currency/reportingCurrency so it computes a filtered list (e.g., filteredExcluded = excludedReportingCurrencies.filter(c => c !== reportingCurrency && c !== currency)) and use filteredExcluded for the displayed list and pluralization logic, while keeping the existing conditional that shows "The revenue chart is showing {currency}." only when currency !== reportingCurrency; this ensures the excluded list and the chart-currency sentence don't contradict each other.
614-640: 💤 Low valueMinor: dead-falsy branches now that
currencyalways resolves.
currencyis now always a truthy string (it falls back toreportingCurrency), socurrency ?· ${currency}: ""(Line 617) andcurrency || undefined(Line 558) can never take the empty path. Not a functional bug — just leftover defensiveness from the earlier nullable model that adds a bit of noise.🤖 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 614 - 640, The subtitle string construction on ChartCard (title "Revenue") still uses a redundant ternary `currency ? \` · ${currency}\` : ""`; replace it with a simple interpolation that always appends ` · ${currency}` (or build the subtitle using currency directly) since currency is always a truthy string; likewise simplify any other redundant guards like `currency || undefined` elsewhere in this file (e.g., where passed into Tooltip/formatters or props) to just pass `currency` to remove dead-falsy branches while preserving the same runtime behavior.packages/kit/convex/subscriptions/query.ts (1)
335-335: 💤 Low valueMinor: double normalization of
reportingCurrency.
reportingCurrencyForProject(project)(Line 335) already callsnormalizeReportingCurrencyOrDefault, andselectReportingMrrre-normalizes itsreportingCurrencyargument internally (Line 90-91). The work is idempotent, so this is purely cosmetic. IfselectReportingMrris intended as a standalone exported API (as the test file suggests), the inner normalization is the right place to keep it — at which point the redundantreportingCurrencyForProjecthelper inside this file mostly exists to give the local variable a name. Worth considering inlining or dropping the helper.Also applies to: 466-466
🤖 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` at line 335, The code double-normalizes reportingCurrency: reportingCurrencyForProject(project) already calls normalizeReportingCurrencyOrDefault and selectReportingMrr also normalizes its input; keep normalization inside selectReportingMrr and remove the redundant outer normalization at the call sites (replace uses of reportingCurrencyForProject(project) with the raw project.reportingCurrency or pass project.reportingCurrency directly into selectReportingMrr), and make the same change for the other occurrence mentioned; adjust or remove the reportingCurrency local variable so callers no longer pre-normalize before calling selectReportingMrr.
🤖 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 80-103: Convert the exported MrrCurrencyEntry type alias to an
exported interface (interface MrrCurrencyEntry { currency: string; mrrMicros:
number; }) and add JSDoc above the exported selectReportingMrr function
documenting parameters and return shape, including that reportingCurrency is
normalized via normalizeReportingCurrencyOrDefault, that a missing match yields
mrrMicros = 0, and that non-reporting entries are returned in
excludedMrrByCurrency; ensure the JSDoc names the returned fields (currency,
mrrMicros, excludedMrrByCurrency) and describes their semantics.
In `@packages/kit/src/pages/auth/organization/project/settings.tsx`:
- Around line 718-734: The input lacks an aria-describedby linking it to the
error message, so add an id for the error node (e.g., reportingCurrencyErrorId)
and set the input's aria-describedby to that id only when
isReportingCurrencyValid is false; update the <p> error element to include that
id and ensure the input retains aria-invalid={!isReportingCurrencyValid} and
other props (use the existing reportingCurrency state, setReportingCurrency
handler, and isReportingCurrencyValid flag to control when the aria-describedby
is present).
---
Nitpick comments:
In `@packages/kit/convex/subscriptions/query.ts`:
- Line 335: The code double-normalizes reportingCurrency:
reportingCurrencyForProject(project) already calls
normalizeReportingCurrencyOrDefault and selectReportingMrr also normalizes its
input; keep normalization inside selectReportingMrr and remove the redundant
outer normalization at the call sites (replace uses of
reportingCurrencyForProject(project) with the raw project.reportingCurrency or
pass project.reportingCurrency directly into selectReportingMrr), and make the
same change for the other occurrence mentioned; adjust or remove the
reportingCurrency local variable so callers no longer pre-normalize before
calling selectReportingMrr.
In `@packages/kit/src/lib/utils.test.ts`:
- Around line 5-13: Add unit tests for normalizeCurrencyCode to cover null and
undefined inputs and the default-fallback path: write tests that call
normalizeCurrencyCode(null) and normalizeCurrencyCode(undefined) asserting they
return the provided fallback or DEFAULT_REPORTING_CURRENCY when no fallback arg
is passed, and add a test that calls normalizeCurrencyCode(undefined, "GBP") to
verify explicit fallback behavior; reference the normalizeCurrencyCode function
and DEFAULT_REPORTING_CURRENCY constant in your new test cases.
In `@packages/kit/src/lib/utils.ts`:
- Around line 23-34: The options object type for formatMicros is currently an
inline anonymous type; extract it to a named exported interface (e.g.,
FormatMicrosOptions) and replace the inline type in the formatMicros signature
with that interface, preserving the same optional properties (currency?: string
| null, compact?: boolean, emptyWhenZero?: boolean) and default parameter value
handling; update any imports/exports so the new interface is exported if used
externally and ensure callers continue to work without code changes.
In `@packages/kit/src/pages/auth/organization/project/analytics.tsx`:
- Around line 570-579: The callout should not list the active chart currency as
an "excluded" currency; update the JSX block that references
excludedReportingCurrencies/currency/reportingCurrency so it computes a filtered
list (e.g., filteredExcluded = excludedReportingCurrencies.filter(c => c !==
reportingCurrency && c !== currency)) and use filteredExcluded for the displayed
list and pluralization logic, while keeping the existing conditional that shows
"The revenue chart is showing {currency}." only when currency !==
reportingCurrency; this ensures the excluded list and the chart-currency
sentence don't contradict each other.
- Around line 614-640: The subtitle string construction on ChartCard (title
"Revenue") still uses a redundant ternary `currency ? \` · ${currency}\` : ""`;
replace it with a simple interpolation that always appends ` · ${currency}` (or
build the subtitle using currency directly) since currency is always a truthy
string; likewise simplify any other redundant guards like `currency ||
undefined` elsewhere in this file (e.g., where passed into Tooltip/formatters or
props) to just pass `currency` to remove dead-falsy branches while preserving
the same runtime behavior.
🪄 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: 3582773c-b131-47f3-925c-65671e26fbf6
📒 Files selected for processing (11)
packages/kit/convex/projects/mutation.tspackages/kit/convex/schema.tspackages/kit/convex/subscriptions/query.test.tspackages/kit/convex/subscriptions/query.tspackages/kit/convex/utils/currency.tspackages/kit/src/lib/utils.test.tspackages/kit/src/lib/utils.tspackages/kit/src/pages/auth/organization/project/analytics.tsxpackages/kit/src/pages/auth/organization/project/settings.tsxpackages/kit/src/pages/auth/organization/project/subscriptions.tsxpackages/kit/src/pages/docs/sections/analytics.tsx
|
/gemini review |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Code Review
This pull request introduces a project-level reporting currency setting to ensure consistent MRR and revenue totals in the dashboard without performing automatic FX conversion. It includes updates to the Convex schema, mutations for project creation and updates, and utility functions for currency normalization and formatting. The analytics and subscription pages now default to the reporting currency while allowing users to explore other currency slices. A review comment suggests expanding the test coverage for the formatMicros utility to include values between 10 and 1000 when using compact formatting.
|
@coderabbitai review |
|
/gemini review |
✅ Actions performedReview triggered.
|
|
Warning Gemini is experiencing higher than usual traffic and was unable to create the review. Please try again in a few hours by commenting |
Summary
Closes #135
Test plan
bun run --filter @hyodotdev/openiap-kit lintbun run --filter @hyodotdev/openiap-kit format:checkbun run --filter @hyodotdev/openiap-kit testbun run --filter @hyodotdev/openiap-kit smoke:serverbun audit:docs(advisory warnings only; no hard failures)Summary by CodeRabbit
New Features
Documentation