Feat/featrue gating and quota enforcement - #15
Conversation
📝 WalkthroughWalkthroughAdds tenant quota enforcement with monthly usage tracking, Arcjet token-bucket configuration, required Arcjet environment validation, runtime dependencies, CI/test environment values, and unit tests covering quota and rate-limit behavior. ChangesTenant quota and rate limiting
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant QuotaService
participant Tenant
participant TenantUsage
QuotaService->>Tenant: Load tenant quotas
QuotaService->>TenantUsage: Read current monthly usage
TenantUsage-->>QuotaService: Return event count
QuotaService-->>QuotaService: Return quota decision
QuotaService->>TenantUsage: Increment monthly event count
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@lib/arcjet.ts`:
- Around line 15-25: Replace the async per-tenant max callback in the
tokenBucket configuration with a static integer max value, and remove the
associated as any cast. Preserve tenant-specific quota lookup only where
supported, but ensure tokenBucket receives a numeric max directly.
In `@lib/config/index.ts`:
- Around line 22-23: Ensure Vitest/CI supplies valid mock values for the
required ARCJET_KEY and ARCJET_ENV variables before getServerConfig()
initializes, using the existing test setup or environment configuration. Keep
the production schema requirements unchanged and ensure the values satisfy the
Zod constraints.
In `@services/quota-tenant.service.ts`:
- Around line 143-146: Remove the redundant tenantId parameter from
QuotaTenantService.incrementEventCount and update its findOneAndUpdate query and
log.error payload to use this.tenantId. Apply the signature change at
services/quota-tenant.service.ts lines 143-146 and the instance-property
replacements at lines 151-161.
🪄 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: 1205f56d-6b7a-4fa6-9642-00b32bb50645
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
__tests__/unit/quota-service.test.tslib/arcjet.tslib/config/index.tslib/database/models/tenant-usage.model.tspackage.jsonservices/quota-tenant.service.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/quota-tenant.service.ts (1)
101-106: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftPer-tenant API rate limit is resolved but never enforced.
getApiRateLimit()computes a tenant-specific limit, yet nothing actually applies it:checkApiRateLimitalways returnsallowed: trueregardless of the limit, and Arcjet'stokenBucketuses one fixed globalcapacityfor every tenant. Given this PR's goal of tenant quota enforcement, API rate limiting is currently a no-op for all tenants.
services/quota-tenant.service.ts#L101-L106: MakecheckApiRateLimitactually compare a request/usage count againstlimitand returnallowed: falsewhen exceeded, instead of always returningallowed: true.lib/arcjet.ts#L4-L16: Either accept that Arcjet enforces only a shared floor/ceiling and rely oncheckApiRateLimit(once fixed) for per-tenant enforcement, or use Arcjet's supported mechanism (e.g., acharacteristics-scoped rule per tenant) to vary capacity — the current staticcapacity: 100combined with the leftover TODO comment indicates this was never completed.🤖 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 `@services/quota-tenant.service.ts` around lines 101 - 106, The per-tenant API limit is resolved but never enforced. In services/quota-tenant.service.ts lines 101-106, update checkApiRateLimit to compare the current request or usage count against limit and return allowed: false once exceeded, preserving the limit in the result. In lib/arcjet.ts lines 4-16, either explicitly retain Arcjet as a shared floor/ceiling while relying on checkApiRateLimit for tenant enforcement and remove the incomplete TODO, or configure a supported tenant-scoped characteristics rule so capacity varies by tenant; do not leave the static global capacity as the sole enforcement.
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
91-92: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: quote
ARCJET_KEYfor consistency.Every other value in this env block is quoted;
ajkey_dummyon Line 92 is bare while the test job's equivalent (Line 47) is quoted.Proposed fix
- ARCJET_KEY: ajkey_dummy + ARCJET_KEY: "ajkey_dummy"🤖 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 @.github/workflows/ci.yml around lines 91 - 92, Quote the ARCJET_KEY value in the environment block for consistency with the surrounding variables and the equivalent test-job configuration, without changing its value.
🤖 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 `@services/quota-tenant.service.ts`:
- Around line 148-158: Update the increment flow around
TenantUsage.findOneAndUpdate so database failures are surfaced to callers
instead of being swallowed after logging. Rethrow the caught error, or propagate
an equivalent failure indicator, and ensure callers such as canTrackEvent can
fail closed or otherwise handle an unsuccessful counter update.
---
Outside diff comments:
In `@services/quota-tenant.service.ts`:
- Around line 101-106: The per-tenant API limit is resolved but never enforced.
In services/quota-tenant.service.ts lines 101-106, update checkApiRateLimit to
compare the current request or usage count against limit and return allowed:
false once exceeded, preserving the limit in the result. In lib/arcjet.ts lines
4-16, either explicitly retain Arcjet as a shared floor/ceiling while relying on
checkApiRateLimit for tenant enforcement and remove the incomplete TODO, or
configure a supported tenant-scoped characteristics rule so capacity varies by
tenant; do not leave the static global capacity as the sole enforcement.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 91-92: Quote the ARCJET_KEY value in the environment block for
consistency with the surrounding variables and the equivalent test-job
configuration, without changing its value.
🪄 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: 13788ca6-77a1-4973-88ad-005ce6a22666
📒 Files selected for processing (4)
.github/workflows/ci.ymllib/arcjet.tsservices/quota-tenant.service.tsvitest.config.ts
| try { | ||
| await TenantUsage.findOneAndUpdate( | ||
| { tenantId: this.tenantId, year, month }, | ||
| { $inc: { count: amount } }, | ||
| { upsert: true, new: true }, | ||
| ); | ||
| } catch (error) { | ||
| log.error("Failed to increment event count", error as Error, { | ||
| tenantId: this.tenantId, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Swallowed increment errors can silently bypass quota enforcement.
If the findOneAndUpdate upsert fails, the error is only logged — the caller (and thus canTrackEvent's subsequent reads) has no way to know the usage counter wasn't updated. Since this counter is the sole source of truth for canTrackEvent, transient DB failures let a tenant's real usage silently drift below what's tracked, undermining the quota it enforces.
Consider surfacing the failure (rethrow, or return a success/failure indicator) so callers can decide whether to fail closed for quota-sensitive paths.
🤖 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 `@services/quota-tenant.service.ts` around lines 148 - 158, Update the
increment flow around TenantUsage.findOneAndUpdate so database failures are
surfaced to callers instead of being swallowed after logging. Rethrow the caught
error, or propagate an equivalent failure indicator, and ensure callers such as
canTrackEvent can fail closed or otherwise handle an unsuccessful counter
update.
1 participant
Summary by CodeRabbit