You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
@PierreBrisorgueil has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 35 minutes and 51 seconds before requesting another review.
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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.
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.
This PR consolidates billing configuration reading into centralized utility functions, adds startup validation for alert thresholds, introduces jitter to the weekly reset cron, strengthens stepKey validation in meter attribution, and expands test coverage for all changes.
Added getDollarsToUnitRatio(), getMaxUnitsPerOperation(), and getDefaultPlanId() functions to centralize billing config reading with defined fallbacks.
Documentation modules/billing/README.md
Added "Meter hardening configuration" section with tabular reference of configuration knobs and their defaults.
Added boot-time validation of alert threshold percents against supported set {80, 100}, emitting warnings for unsupported values.
Service Updates modules/billing/services/billing.meter.service.js, modules/billing/services/billing.reset.service.js, modules/billing/services/billing.usage.service.js
Updated to use new config utility functions instead of direct config.billing reads; billing.meter.service.js now validates stepKey strictly when non-null.
Added tests covering new config functions, alert threshold validation, jitter registration, usage threshold regression, and refactored imports (isoWeekKey import consolidation in lifecycle and reset service tests).
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
PR#3536: Directly overlaps—both modify billing constants, meter service, reset service, and usage service to add/configure meter/plan defaults and config-driven behavior.
PR#3582: Overlaps in billing reset and meter services, including plan-fallback and attribute/usage behavior changes.
PR#3579: Related through jitter and sharding documentation for the same weekly reset cron behavior.
The title 'fix(billing): v4 hardening — finish v3 oversights + cohérence API' clearly summarizes the main change: completing v4 billing hardening with multiple polish items and API coherence improvements.
Description check
✅ Passed
The description covers the key required sections: Summary (what changed and context), detailed changes with categorization, and test plan. All critical information is present.
Docstring Coverage
✅ Passed
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check
✅ Passed
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
✅ Passed
Check skipped because no linked issues were found for this pull request.
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches🧪 Generate unit tests (beta)
Create PR with unit tests
Commit unit tests in branch fix/billing-hardening-v4
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.
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
The PR successfully implements critical hardening for the billing module, specifically addressing cron jitter and threshold event logic. Codacy results indicate the PR is up to standards, though there is a noticeable increase in code duplication across cron scripts.
One significant logic issue remains: while the removal of the loop break in billing.usage.service.js correctly allows multiple threshold events to fire, the function's return value logic is flawed. When multiple thresholds are crossed (e.g., jumping from 0% to 150%), the current implementation returns the lowest threshold (80%) rather than the most critical one (100%). This should be corrected before merging to ensure calling services receive the correct status signal.
1 comment outside of the diffmodules/billing/services/billing.usage.service.js
line 154 🟡 MEDIUM RISK
The removal of the break statement correctly ensures that all crossed thresholds (e.g., 80% and 100%) are processed. However, because the thresholds are sorted descending, the alertCrossed variable is being overwritten by lower values in subsequent iterations. It should be locked to the highest (first) threshold crossed to ensure the caller receives the most urgent signal.
if (marked && !alertCrossed) {
alertCrossed = String(threshold);
}
Test suggestions
Weekly reset cron correctly imports and executes jitter logic
Usage attribution jump from 0% to 150% triggers both 80% and 100% threshold events sequentially
Module initialization warns when alert thresholds contain unsupported values like 75% while meterMode is enabled
Billing constants (dollarsToUnitRatio, maxUnitsPerOperation, defaultPlan) resolve with correct defaults when config is missing
isoWeekKey computes correct ISO week strings for Monday, Sunday, and year-start boundaries
Low confidence findings
Systemic pattern: The import and initialization block for cron entry points is duplicated across weeklyReset, dunningSweep, and extrasExpiration. Consider centralizing this 'boot' logic into a reusable utility (e.g., modules/billing/lib/billing-cron-init.js) to simplify future updates to module structure or core services.
✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.26%. Comparing base (5f12d2a) to head (a5b5329). ⚠️ Report is 1 commits behind head on master.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/billing/lib/billing.errors.js (1)
3-11: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
Potential bug: isDuplicateKeyError doesn’t detect raw string errors.
Your doc says it should work for “string-only shapes”, but the implementation only checks err?.message. If a caller ever passes a raw string (e.g., isDuplicateKeyError("E11000 ...")), it will return false.
Verify each finding against the current code and only fix it if needed.
In `@modules/billing/lib/billing.errors.js` around lines 3 - 11,
isDuplicateKeyError currently only checks err?.code and err?.message which
misses callers that pass a raw string; update the function isDuplicateKeyError
to also handle when err itself is a string by checking typeof err === 'string'
and searching for 'E11000' (in addition to the existing err?.message check)
while preserving the numeric code check (err?.code === 11000) and safe
null/undefined handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@modules/billing/lib/billing.constants.js`:
- Around line 92-109: Validate and sanitize the billing getters before returning
config values: in getDollarsToUnitRatio ensure the returned ratio is a positive
finite number (fallback to 1000 if value is 0, negative, NaN, or not present),
in getMaxUnitsPerOperation ensure the value is either positive finite or
Infinity (fallback to Infinity on 0, negative, NaN, or missing), and in
getDefaultPlanId ensure the plan id is a non-empty string (fallback to 'free' if
empty, not a string, or missing); implement these checks inside
getDollarsToUnitRatio, getMaxUnitsPerOperation, and getDefaultPlanId
respectively so callers always receive safe, expected types and ranges.
In `@modules/billing/README.md`:
- Line 125: The README incorrectly points only to
modules/billing/config/billing.development.config.js as the source of defaults;
update the sentence to reference modules/billing/lib/billing.constants.js as the
source for defaults resolved there (specifically DEFAULT_CRON_JITTER_MAX_MS,
DEFAULT_ALERT_THRESHOLD_PERCENTS, getDollarsToUnitRatio(),
getMaxUnitsPerOperation(), getDefaultPlanId()) and note that downstream projects
can still override via modules/billing/config/billing.development.config.js so
readers see both the canonical default implementations and the override
location.
In `@modules/billing/services/billing.meter.service.js`:
- Around line 44-45: The code reads numeric knobs like dollarsToUnitRatio (from
getDollarsToUnitRatio()) and maxUnitsPerOperation and uses them directly for
billing math, which can produce 0/NaN/negative results; add validation after
reading these resolver values to fail fast: validate dollarsToUnitRatio is a
finite positive number and maxUnitsPerOperation is a finite positive integer (or
at least > 0), throw or log an error and abort processing if checks fail, and
sanitize values before calling incrementMeterWithOutbox() (e.g., clamp or reject
invalid inputs) so invalid configs cannot produce zero/NaN units or propagate
bad values into incrementMeterWithOutbox.
In `@modules/billing/services/billing.service.js`:
- Around line 196-197: The idempotencyKey currently uses Date.now() which can
differ across rapid retries; replace it with a stable purchase-intent identifier
so Stripe can deduplicate retries. Change the idempotencyKey construction
(variable idempotencyKey in billing.service.js) to use a caller-provided
intent/request id (e.g., request.intentId) or a server-issued nonce that is
persisted and returned to the caller and reused on retry, keeping the
organization._id and packId context in the key; ensure whatever function that
calls Stripe (the extras checkout flow around idempotencyKey) reads this stable
identifier and uses it in the Stripe request instead of Date.now().
In `@modules/billing/tests/billing.cron.weeklyReset.unit.tests.js`:
- Around line 14-24: Change the tests to exercise the actual cron entrypoint in
billing.weeklyReset.js: import the module (e.g. await
import('../lib/billing.weeklyReset.js')) and mock the helper functions and
mongoose connect so you can assert call order; specifically spy/mock
billing.cron-utils.applyJitter to return a resolved promise, mock
billing.constants.getCronJitterMaxMs to return a fixed number, and mock
mongooseService.connect to be a jest mock that records when it was called, then
invoke the cron entrypoint and assert that applyJitter was awaited (called
before mongooseService.connect) by checking the call order or timestamps (i.e.,
expect(applyJitter).toHaveBeenCalled();
expect(mongooseService.connect).toHaveBeenCalled();
expect(applyJitter).toHaveBeenCalledBefore(mongooseService.connect) or assert
call indices).
In `@modules/billing/tests/billing.usage.service.unit.tests.js`:
- Around line 418-441: Add assertions that the service actually emitted two
meter.threshold_crossed events when jumping from 0%→150%; after calling
BillingUsageService.incrementMeter, assert the event emitter was called twice
(expect(mockEventEmitter.emit).toHaveBeenCalledTimes(2)) and that the calls
include meter.threshold_crossed with the 100 threshold payload first and then 80
(use expect(mockEventEmitter.emit).toHaveBeenNthCalledWith(1,
'meter.threshold_crossed', expect.objectContaining({ threshold: '100' })) and
NthCalledWith(2, 'meter.threshold_crossed', expect.objectContaining({ threshold:
'80' }))). Also ensure you reference the same mockEventEmitter used by the
service and keep the existing markThreshold assertions for coverage.
---
Outside diff comments:
In `@modules/billing/lib/billing.errors.js`:
- Around line 3-11: isDuplicateKeyError currently only checks err?.code and
err?.message which misses callers that pass a raw string; update the function
isDuplicateKeyError to also handle when err itself is a string by checking
typeof err === 'string' and searching for 'E11000' (in addition to the existing
err?.message check) while preserving the numeric code check (err?.code ===
11000) and safe null/undefined handling.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bfab9ba4-6c14-4ffc-a872-c799bf57a9a2
📥 Commits
Reviewing files that changed from the base of the PR and between 5f12d2a and 50be39d.
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR applies another round of billing v4 hardening to finish post-v3 cleanup: it tightens meter-threshold behavior, centralizes a few remaining billing config lookups, adds weekly-reset cron jitter, and updates tests/docs to match the intended API surface.
Changes:
Adds/finishes billing config helpers (getDefaultPlanId, getDollarsToUnitRatio, getMaxUnitsPerOperation) and switches services to use them.
Adjusts metering/reset behavior around threshold emission and isoWeekKey usage/export boundaries.
Expands unit/integration coverage and updates billing docs/config references.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 5 comments.
Math.random() in the idempotency key was flagged by Codacy as insecure
random in a security-sensitive context. Switch to node:crypto randomBytes(4)
which produces a cryptographically secure 8-char hex suffix.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![codacy-production[bot]](https://avatars.githubusercontent.com/in/56611?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Summary
8 v4 polish items finishing what v3 mega started. 100% backward compat. Source: audit V3 post-mega 4-agents (2026-05-03).
Changes
Test plan
Summary by CodeRabbit
Documentation
Tests
Chores