Skip to content

fix: stripe addon - #2294

Merged
mxkaske merged 4 commits into
mainfrom
fix/stripe-add-on
Jun 20, 2026
Merged

fix: stripe addon#2294
mxkaske merged 4 commits into
mainfrom
fix/stripe-add-on

Conversation

@mxkaske

@mxkaske mxkaske commented Jun 19, 2026

Copy link
Copy Markdown
Member

Review in cubic

@vercel

vercel Bot commented Jun 19, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
openstatus-dashboard Ready Ready Preview, Comment Jun 20, 2026 7:35am
openstatus-status-page Ready Ready Preview, Comment Jun 20, 2026 7:35am
openstatus-web Ready Ready Preview, Comment Jun 20, 2026 7:35am

Request Review

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 issues found across 4 files

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread packages/api/src/router/stripe/utils.ts Outdated
Comment thread packages/api/src/router/stripe/webhook.ts
@openstatusHQ openstatusHQ deleted a comment from claude Bot Jun 20, 2026
@mxkaske

mxkaske commented Jun 20, 2026

Copy link
Copy Markdown
Member Author

@claude review

@claude

claude Bot commented Jun 20, 2026

Copy link
Copy Markdown

Claude finished @mxkaske's task in 2m 4s —— View job


PR Review

  • Gather context and understand the request
  • Read changed files
  • Analyze changes and identify issues
  • Post review findings

Summary

This PR centralizes the "rebuild workspace limits from Stripe subscription" logic into a single testable function buildLimitsFromSubscription in utils.ts, replacing scattered inline logic in webhook.ts. The primary bug fixed is that addon limits were being reset to plan defaults on subscription updates instead of being preserved. The implementation is clean and the test suite covers all the important cases.

Overall: Looks good with a few things worth flagging.


Cubic Issue #2 — Is it actually unresolved?

sessionCompleted no longer validates unknown subscription line-item prices when a plan exists.

This appears to be a false alarm on the current code. buildLimitsFromSubscription in utils.ts:37-40 explicitly throws for any price that is neither a known plan nor a known addon:

if (!feature) {
  throw new Error(
    `Unsupported Stripe price on subscription: ${item.price.id}`,
  );
}

The JSDoc at the top of the function also documents this invariant. Cubic may have flagged an intermediate commit before the throw was added.


Issues Found

[P2] getFeatureFromPriceId only looks at monthly price IDs (utils.ts:67)

return FEATURES.find(
  (feature) => feature.price.monthly.priceIds[env] === priceId,
);

If a yearly addon price is ever introduced (or already exists on Stripe), it won't match here and will hit the throw new Error("Unsupported Stripe price") branch, causing webhook failures at the next subscription update. The monthly-only lookup is unintentional, given that getPlanFromPriceId checks both monthly and yearly:

plan.price.monthly.priceIds[env] === priceId ||
plan.price.yearly.priceIds[env] === priceId,

The FEATURES data structure only has monthly pricing today so this won't break right now, but the asymmetry is fragile. Fix this →


[P2] Behavior asymmetry between the two webhook handlers (webhook.ts:67 vs webhook.ts:156)

When buildLimitsFromSubscription returns null (subscription has no recognized plan item):

  • customerSubscriptionUpdated: silently returns early (line 67–69)
  • sessionCompleted: throws TRPCError BAD_REQUEST "Invalid plan" (line 156–161)

This is probably the right behavior — sessionCompleted always follows a checkout flow so a plan must be present, while customerSubscriptionUpdated may fire on non-plan subscriptions legitimately. A short comment on the early return would save future readers from second-guessing the asymmetry.


[P3] Unknown prices propagate as unhandled exceptions in customerSubscriptionUpdated

When buildLimitsFromSubscription throws (e.g., a price not in PLANS or FEATURES), the error will propagate as an INTERNAL_SERVER_ERROR through tRPC. Stripe will retry the webhook. Depending on your Stripe webhook config, this could hammer the endpoint. Both handlers would benefit from either:

  1. A try/catch that logs and explicitly returns a 400 to stop Stripe retries for a misconfiguration, or
  2. A comment acknowledging that propagation + retry is the intended behavior.

[Nit] Debug console.log in production code (index.ts:102)

console.log("getCheckoutSession");

This is unrelated to this PR but worth cleaning up — it'll appear in Vercel/production logs on every checkout session creation.


What's Done Well

  • Accumulation fix is correct. limits[feature.feature] reads the running accumulated value (not the plan baseline each loop), so repeated addon items correctly stack.
  • Tests are excellent. The test file derives price IDs from the source tables (planPriceId / featurePriceId), so any future price ID changes will break the tests loudly. Every behavior path is covered: null, baseline, boolean addon, quantity addon, accumulation, and the throw case.
  • Duplicate subscription cancellation logic in customerSubscriptionUpdated is preserved and still runs after the limit update — good.
  • The function returning null instead of throwing when no plan is present is the right design choice; it lets callers decide policy.

@mxkaske

mxkaske commented Jun 20, 2026

Copy link
Copy Markdown
Member Author

@claude review

@claude

claude Bot commented Jun 20, 2026

Copy link
Copy Markdown

Claude finished @mxkaske's task in 2m 3s —— View job


PR Review

  • Gather context and understand the request
  • Read changed files
  • Analyze changes and identify issues
  • Post review findings

Summary

This PR extracts buildLimitsFromSubscription in utils.ts as the single authoritative way to derive workspace limits from a Stripe subscription, fixing the original bug where addon limits were reset to plan defaults on every subscription update. The implementation is correct and the previous review issues have all been addressed.

Overall: Looks good to merge.


What's Fixed and Working Well

Accumulation bug is correctly resolved. The key change is reading limits[feature.feature] (the running accumulated value) instead of the plan baseline on each loop iteration (utils.ts:44). For repeated addon line items, values correctly stack up. The test at utils.test.ts:82-92 covers exactly this case.

getFeatureFromPriceId yearly support — the previous review flagged this as a P2 concern (monthly-only lookup). The new code at utils.ts:66-68 uses Object.values(feature.price).some(...), which iterates all keys of feature.price and will correctly match yearly prices if they're ever added to FEATURES. This is the right approach.

buildFromSubscriptionOrThrow wrapper (webhook.ts:24-34) cleanly addresses the Stripe retry concern from the previous review. Unknown prices now return a 400 (not a 500), which tells Stripe to stop retrying on permanent misconfigurations. The comment at line 23 explains the intent.

Tests are well-structured. Deriving price IDs from the source tables (planPriceId / featurePriceId) means any future price ID change will break the tests loudly — a good invariant.

console.log("getCheckoutSession") has been removed from index.ts. Good cleanup.


Issues Found

[Nit] Redundant console.error before throw in sessionCompleted (webhook.ts:173)

const built = buildFromSubscriptionOrThrow(subscription);
if (!built) {
  console.error("Invalid plan");   // <-- redundant
  throw new TRPCError({ code: "BAD_REQUEST", message: "Invalid plan" });
}

buildFromSubscriptionOrThrow already calls console.error(e) inside the wrapper when it catches a thrown error. The !built branch here handles the null case (no plan item in subscription), which is a business rule violation — the log is slightly misleading since it's not really an error condition that needs logging. Minor but noisy in production logs. Fix this →


[Observation] addAddon in index.ts and buildLimitsFromSubscription are consistent — worth confirming for reviewers. addAddon sets the Stripe subscription item quantity to opts.input.value, then stores value + planDefault locally. buildLimitsFromSubscription reads the subscription item quantity Q and computes planDefault + Q. These are equivalent when Q === value. The webhook path is the authoritative source of truth, and the local update in addAddon is just an optimistic sync until the webhook fires. No issue here.


Addressed Since Previous Review

Issue Status
Accumulation reads plan baseline each loop (Cubic P2) ✅ Fixed in cef24b8
Unknown prices silently ignored (Cubic P2) ✅ Fixed — throws, wrapped as BAD_REQUEST
getFeatureFromPriceId monthly-only lookup ✅ Fixed — uses Object.values
Unknown price causes 500 + Stripe retry loop ✅ Fixed — buildFromSubscriptionOrThrow returns 400
console.log("getCheckoutSession") in production ✅ Removed

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 3 files (changes from recent commits).

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread packages/api/src/router/stripe/webhook.ts
@mxkaske
mxkaske merged commit 7a84903 into main Jun 20, 2026
15 checks passed
@mxkaske
mxkaske deleted the fix/stripe-add-on branch June 20, 2026 07:39
psdojo pushed a commit to psdojo/openstatus that referenced this pull request Jul 5, 2026
* fix: stripe addon

* fix: review

* wip:

* fix: review
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant