fix: disable auto top-up after 7 days - #1918
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughThe changes implement automatic disablement of auto top-ups after 7 days of persistent payment failures. A new Changes
Sequence DiagramsequenceDiagram
participant Worker as Worker Process
participant Org as Organization DB
participant AuditLog as Audit Log
Worker->>Org: Query organizations with paymentFailureStartedAt set
Org-->>Worker: Return orgs in failure state
Worker->>Worker: Check if (now - paymentFailureStartedAt) >= 7 days
alt Failure Threshold Exceeded
Worker->>Org: Disable autoTopUpEnabled, reset failure counts/timestamps
Worker->>AuditLog: Create payment.auto_topup.disable entry with failure metadata
AuditLog-->>Worker: Audit logged
Worker->>Worker: Emit warning, continue to next org
else Within 7-Day Window
Worker->>Worker: Preserve failure state, process auto-topup normally
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f65e3b3dc6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| referralEarnings: decimal().notNull().default("0"), | ||
| paymentFailureCount: integer().notNull().default(0), | ||
| lastPaymentFailureAt: timestamp(), | ||
| paymentFailureStartedAt: timestamp(), |
There was a problem hiding this comment.
This patch adds paymentFailureStartedAt to the Drizzle schema, but no new file under packages/db/migrations/ is included to actually add payment_failure_started_at in existing databases. In environments that rely on migration files (e.g., runMigrations() in packages/db/src/migrate.ts, invoked from apps/api/src/serve.ts when RUN_MIGRATIONS=true), the new worker/API queries in this commit will hit column does not exist at runtime when reading/updating organization records.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Adds “payment failure streak start” tracking and uses it to automatically disable auto top-up after 7 days of continued payment failures, with resets on successful top-up or manual re-enable.
Changes:
- Add
paymentFailureStartedAtto theorganizationschema/type surface. - Track/reset
paymentFailureStartedAtin Stripe webhook handling and org updates. - Update the worker’s auto top-up loop to disable auto top-up after 7 days of failures, and add worker test coverage for this behavior.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/db/src/types.ts | Excludes paymentFailureStartedAt from SerializedOrganization like other internal billing fields. |
| packages/db/src/schema.ts | Adds paymentFailureStartedAt column to organization. |
| apps/worker/src/worker.ts | Disables auto top-up after 7 days since paymentFailureStartedAt; exports processAutoTopUp for tests. |
| apps/worker/src/worker.spec.ts | Adds tests covering the 7-day disable behavior and non-disable behavior before 7 days. |
| apps/gateway/src/lib/rate-limit.spec.ts | Updates mocked organization objects to include paymentFailureStartedAt. |
| apps/api/src/stripe.ts | Resets paymentFailureStartedAt on success; sets it on payment failures. |
| apps/api/src/routes/organization.ts | Resets failure tracking when auto top-up is manually re-enabled. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| referralEarnings: decimal().notNull().default("0"), | ||
| paymentFailureCount: integer().notNull().default(0), | ||
| lastPaymentFailureAt: timestamp(), | ||
| paymentFailureStartedAt: timestamp(), |
There was a problem hiding this comment.
The schema adds organization.paymentFailureStartedAt, and application code now reads/writes this column (worker + Stripe webhook). There is no corresponding SQL migration in packages/db/migrations, so deployments will fail at runtime with "column does not exist" errors. Add a migration that adds this column (and consider backfilling it for existing orgs with paymentFailureCount > 0).
| @@ -1035,6 +1038,7 @@ async function handlePaymentIntentFailed( | |||
| .set({ | |||
| paymentFailureCount: newFailureCount, | |||
| lastPaymentFailureAt: new Date(), | |||
| paymentFailureStartedAt: failureStartedAt, | |||
| }) | |||
There was a problem hiding this comment.
failureStartedAt currently falls back to new Date() when paymentFailureStartedAt is null. For existing rows (new column) that already have paymentFailureCount > 0/lastPaymentFailureAt populated, this effectively “restarts” the streak on the next failure and can delay auto top-up disabling. Consider falling back to organization.lastPaymentFailureAt (if present) before new Date() so the streak start is preserved as best as possible for pre-existing data.
| if ( | ||
| org.paymentFailureStartedAt && | ||
| Date.now() - org.paymentFailureStartedAt.getTime() >= |
There was a problem hiding this comment.
The disable-after-7-days check relies solely on org.paymentFailureStartedAt. For existing organizations that already have paymentFailureCount > 0/lastPaymentFailureAt set (from before this column existed), paymentFailureStartedAt will be null and auto top-up will never be disabled until another failure occurs. Consider initializing/backfilling paymentFailureStartedAt (e.g., from lastPaymentFailureAt) when it’s missing but failures are present, or ensure the migration backfills it.
| if ( | |
| org.paymentFailureStartedAt && | |
| Date.now() - org.paymentFailureStartedAt.getTime() >= | |
| // Backfill paymentFailureStartedAt for legacy organizations if needed | |
| let paymentFailureStartedAt = org.paymentFailureStartedAt; | |
| if ( | |
| !paymentFailureStartedAt && | |
| org.paymentFailureCount > 0 && | |
| org.lastPaymentFailureAt | |
| ) { | |
| await db | |
| .update(tables.organization) | |
| .set({ | |
| paymentFailureStartedAt: org.lastPaymentFailureAt, | |
| }) | |
| .where(eq(tables.organization.id, org.id)); | |
| paymentFailureStartedAt = org.lastPaymentFailureAt; | |
| } | |
| if ( | |
| paymentFailureStartedAt && | |
| Date.now() - paymentFailureStartedAt.getTime() >= |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/worker/src/worker.spec.ts`:
- Line 128: Wrap the multiplication expression used for the date math in the
paymentFailureStartedAt assignment (and the similar expression around line 167)
with parentheses so the subtraction and multiplication operators are explicitly
grouped; for example, change new Date(Date.now() - 8 * 24 * 60 * 60 * 1000) to
use new Date(Date.now() - (8 * 24 * 60 * 60 * 1000)) to satisfy the
no-mixed-operators rule and clarify precedence for the paymentFailureStartedAt
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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c193f849-b5fc-4e6d-b4fc-35d77b4bedbc
📒 Files selected for processing (7)
apps/api/src/routes/organization.tsapps/api/src/stripe.tsapps/gateway/src/lib/rate-limit.spec.tsapps/worker/src/worker.spec.tsapps/worker/src/worker.tspackages/db/src/schema.tspackages/db/src/types.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 154efd912a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -0,0 +1 @@ | |||
| ALTER TABLE "organization" ADD COLUMN "payment_failure_started_at" timestamp; | |||
There was a problem hiding this comment.
Adding 1774728743_solid_randall.sql alone is not enough for this repo’s migration flow: in this commit, packages/db/migrations/meta/_journal.json still ends at 1774721979_left_mimic, so the new payment_failure_started_at migration is not tracked/applied on upgraded environments. That leaves the new worker/API updates touching organization.paymentFailureStartedAt vulnerable to column does not exist failures after deploy.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cfa8653d4f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Calculate new failure count and check if we should send an email | ||
| const previousFailureCount = organization.paymentFailureCount ?? 0; | ||
| const previousFailureAt = organization.lastPaymentFailureAt; | ||
| const failureStartedAt = organization.paymentFailureStartedAt ?? new Date(); |
There was a problem hiding this comment.
paymentFailureStartedAt is initialized for every payment_intent.payment_failed event, including non-auto-top-up failures (e.g., manual top-ups or subscription-related payment intents resolved via customer lookup). With this commit’s worker logic (apps/worker/src/worker.ts) disabling auto top-up after 7 days based on this field, unrelated payment failures can now shut off auto top-up for an organization even when auto top-up itself hasn’t been failing. Restrict setting this streak start (and the associated counters used for disablement) to verified auto top-up failures only (such as events carrying the auto top-up transaction metadata).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc9a58eec0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Calculate new failure count and check if we should send an email | ||
| const previousFailureCount = organization.paymentFailureCount ?? 0; | ||
| const previousFailureAt = organization.lastPaymentFailureAt; | ||
| const failureStartedAt = organization.paymentFailureStartedAt ?? new Date(); |
There was a problem hiding this comment.
This assignment updates the 7-day disable timer for every payment_intent.payment_failed event that resolves an organization, not just failed auto top-ups. Fresh evidence in this commit: the transactionId-missing path explicitly handles manual/non-auto payments ("for manual top-ups or payments without transactionId") and still flows into this unconditional paymentFailureStartedAt update, while processAutoTopUp now disables auto top-up once that timestamp is 7 days old. A failed non-auto payment can therefore disable auto top-up even when auto top-up itself was healthy.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/routes/organization.spec.ts`:
- Around line 99-106: The test currently uses toMatchObject which allows extra
fields; change the assertion on orgAuditLogs[0]?.metadata to a strict equality
check so no extra keys (e.g., auto-top-up) can sneak into organization.update.
Replace the toMatchObject call with an exact deep equality (e.g.,
expect(orgAuditLogs[0]?.metadata).toEqual({ changes: { name: { old: "Test
Organization", new: "Renamed Organization" } } })) or equivalent strict check,
ensuring you reference the metadata -> changes -> name structure exactly.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 78f1f1a3-4259-4282-909b-a934353fd3d0
⛔ Files ignored due to path filters (4)
apps/code/src/lib/api/v1.d.tsis excluded by!**/v1.d.tsapps/playground/src/lib/api/v1.d.tsis excluded by!**/v1.d.tsapps/ui/src/lib/api/v1.d.tsis excluded by!**/v1.d.tsee/admin/src/lib/api/v1.d.tsis excluded by!**/v1.d.ts
📒 Files selected for processing (6)
apps/api/src/routes/organization.spec.tsapps/api/src/routes/organization.tsapps/api/src/testing.tsapps/worker/src/worker.spec.tsapps/worker/src/worker.tspackages/db/src/schema.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/db/src/schema.ts
- apps/api/src/routes/organization.ts
- apps/worker/src/worker.spec.ts
- apps/worker/src/worker.ts
| expect(orgAuditLogs[0]?.metadata).toMatchObject({ | ||
| changes: { | ||
| name: { | ||
| old: "Test Organization", | ||
| new: "Renamed Organization", | ||
| }, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Assert exact organization.update change keys to enforce separation.
Line 99 currently uses toMatchObject, which permits extra fields. A regression where auto-top-up fields leak into organization.update would still pass.
🔧 Tighten the assertion
- expect(orgAuditLogs[0]?.metadata).toMatchObject({
- changes: {
- name: {
- old: "Test Organization",
- new: "Renamed Organization",
- },
- },
- });
+ const orgChanges = (orgAuditLogs[0]?.metadata as {
+ changes: Record<string, unknown>;
+ }).changes;
+ expect(Object.keys(orgChanges)).toEqual(["name"]);
+ expect(orgChanges).toMatchObject({
+ name: {
+ old: "Test Organization",
+ new: "Renamed Organization",
+ },
+ });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/routes/organization.spec.ts` around lines 99 - 106, The test
currently uses toMatchObject which allows extra fields; change the assertion on
orgAuditLogs[0]?.metadata to a strict equality check so no extra keys (e.g.,
auto-top-up) can sneak into organization.update. Replace the toMatchObject call
with an exact deep equality (e.g., expect(orgAuditLogs[0]?.metadata).toEqual({
changes: { name: { old: "Test Organization", new: "Renamed Organization" } } }))
or equivalent strict check, ensuring you reference the metadata -> changes ->
name structure exactly.
Summary
Validation
pnpm exec vitest run apps/worker/src/worker.spec.ts --no-file-parallelismpnpm buildpnpm test:unit(fails in unrelated existing suites, includingapps/gateway/src/videos/videos.spec.tsandapps/api/src/routes/beacon.spec.tsin this workspace)Summary by CodeRabbit
Release Notes
New Features
Bug Fixes