Add Stripe billing integration#301
Merged
Merged
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughAdds Stripe billing end-to-end: checkout sessions, customer portal, webhook processing, DB schema and accessors for Stripe IDs, API billing endpoints, frontend billing UI and cache handling, 1Password-based local secret injection, and CI/deployment secret wiring for Stripe keys. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (Browser)
participant Settings as Settings UI
participant API as API Handler
participant DB as Database
participant Stripe as Stripe
participant Webhook as Stripe Webhook
User->>Settings: Click "Upgrade"
Settings->>API: POST /v1/billing/checkout (plan_id)
API->>DB: GetPlanByStripePriceID(plan_id)
API->>DB: GetStripeCustomerID(orgID)
alt customer exists
API->>Stripe: Create Checkout Session (customer, price_id)
else
API->>Stripe: Create Customer (org metadata)
Stripe-->>API: customer_id
API->>DB: SetStripeCustomerID(orgID, customer_id)
API->>Stripe: Create Checkout Session (customer, price_id)
end
Stripe-->>API: session.url
API-->>Settings: {url}
Settings->>Stripe: Redirect user to Checkout
Stripe->>Webhook: POST /v1/webhooks/stripe (checkout.session.completed)
Webhook->>Webhook: Verify signature
Webhook->>DB: GetOrganisationIDByStripeCustomerID(customer_id)
Webhook->>DB: SetStripeSubscriptionID(orgID, subscription_id)
Webhook->>DB: GetPlanByStripePriceID(price_id)
Webhook->>DB: Update organisation.plan_id
Webhook-->>Stripe: 200 OK
sequenceDiagram
participant User as User (Browser)
participant Settings as Settings UI
participant API as API Handler
participant DB as Database
participant Stripe as Stripe
participant Webhook as Stripe Webhook
User->>Settings: Click "Switch to Free"
Settings->>Settings: Confirm
Settings->>API: POST /v1/billing/cancel
API->>DB: GetStripeSubscriptionID(orgID)
API->>Stripe: Update subscription (cancel_at_period_end=true)
Stripe-->>API: period_end
API-->>Settings: {period_end}
Stripe->>Webhook: POST /v1/webhooks/stripe (customer.subscription.deleted)
Webhook->>Webhook: Verify signature
Webhook->>DB: GetOrganisationIDByStripeCustomerID(customer_id)
Webhook->>DB: GetFreePlanID()
Webhook->>DB: Update organisation.plan_id -> free
Webhook->>DB: Clear stripe_subscription_id
Webhook-->>Stripe: 200 OK
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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)
Comment |
|
Updates to Preview Branch (worktree-calm-hugging-pebble) ↗︎
Tasks are run on every commit but only new migration files are pushed.
View logs for this Workflow Run ↗︎. |
Contributor
Release VersionsApp minor: ChangelogAdded
Changed
Fixed
|
Contributor
|
🐝 Review App Deployed Homepage: https://hover-pr-301.fly.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/app/main.go`:
- Around line 415-417: Add a consistency check and informational log for missing
Stripe credentials: after the configuration fields StripeSecretKey,
StripeWebhookSecret, and StripePublishableKey are read into config (references:
config.StripeSecretKey, config.StripeWebhookSecret), add a conditional that logs
an informational/warning message via the existing logger (e.g., log.Info().Msg
or log.Warn().Msg) when the secret key or webhook secret are empty so operators
see "Stripe billing unavailable: STRIPE_SECRET_KEY or STRIPE_WEBHOOK_SECRET not
configured" (or similar) in the startup logs.
In `@internal/api/billing.go`:
- Line 69: The code sets the package-global stripe.Key which is not thread-safe;
instead create a per-request Stripe client and use that for API calls. Replace
assignments to stripe.Key with constructing a local client (e.g., using
stripe.NewClient or client.API/stripe.BackendImplementation depending on your
stripe-go version) initialized with h.StripeSecretKey, and update functions that
currently rely on the global to accept and use this client instance (reference
symbols: stripe.Key, stripe.NewClient, client.API, stripe.BackendImplementation,
and any methods that call Stripe APIs in this file) so no global state is
modified.
- Around line 173-180: The Billing Portal ReturnURL may be relative
(h.SettingsURL) but Stripe requires an absolute URL; before calling
portalsession.New with stripe.BillingPortalSessionParams (and customerID),
validate if returnURL is absolute (check scheme/prefix) and if not, construct an
absolute URL using your service base (e.g., parse h.BaseURL with net/url and
ResolveReference against the returnURL or concatenate scheme/host), then assign
the resolved absolute URL back to returnURL so portalsession.New always receives
an absolute ReturnURL.
- Around line 103-124: The SuccessURL and CancelURL passed into
stripe.CheckoutSessionParams are using relative paths (via h.SettingsURL
fallback to "/settings") but Stripe requires fully qualified absolute URLs;
update the code that builds SuccessURL and CancelURL (used when calling
checkoutsession.New with stripe.CheckoutSessionParams) to ensure h.SettingsURL
is an absolute URL (scheme + host) and, if it's a relative value, prepend the
application base URL from configuration/environment before appending the query
string (e.g., build absoluteSuccess := joinBase(baseURL, h.SettingsURL) +
"?billing=success" and likewise for cancel) so the values passed into SuccessURL
and CancelURL are complete URLs.
In `@internal/api/handlers.go`:
- Around line 238-250: NewHandler currently takes many positional parameters
making calls error-prone; introduce a HandlerConfig struct (e.g., HandlerConfig)
that groups DB, JobsManager, Loops and all string fields (GoogleClientID,
GoogleClientSecret, StripeSecretKey, StripeWebhookSecret, StripePublishableKey,
SettingsURL) and change NewHandler to accept that struct (NewHandler(cfg
HandlerConfig)) and populate the returned *Handler from cfg; update all call
sites to construct and pass HandlerConfig instead of the long parameter list.
In `@internal/api/stripe_webhook.go`:
- Around line 162-173: The handler handleSubscriptionDeleted has a nil-pointer
risk when accessing sub.Customer.ID after unmarshalling; update
Handler.handleSubscriptionDeleted to check that sub.Customer is not nil (and
optionally that sub.Customer.ID is non-empty) before using sub.Customer.ID, and
if nil/log an error via logger (e.g., logger.Error().Msg("missing subscription
customer") ) and return early to match the safety in handleSubscriptionUpdated.
- Around line 130-141: In handleSubscriptionUpdated, guard against a nil or
missing Customer on the unmarshaled sub before accessing sub.Customer.ID: check
that sub.Customer is non-nil and that sub.Customer.ID is non-empty, and if it is
nil/empty log an appropriate error (similar to handleInvoicePaymentFailed) and
return instead of calling h.DB.GetOrganisationIDByStripeCustomerID; this
prevents a nil pointer dereference when handling customer.subscription.updated
events.
In `@internal/db/organisations.go`:
- Around line 427-437: SetStripeCustomerID currently executes an UPDATE without
checking whether any rows were affected, so if the organisation ID doesn't exist
the call appears to succeed; modify SetStripeCustomerID to use the Exec result's
RowsAffected() (as done in SetOrganisationPlan) and return a not-found error
when rowsAffected == 0, keeping the existing fmt.Errorf wrapping for other
errors and updating updated_at only when a row is actually changed.
- Around line 474-484: The SetStripeSubscriptionID function currently ignores
whether the UPDATE touched a row; change the ExecContext call in
SetStripeSubscriptionID to capture the sql.Result (e.g., result, err :=
db.client.ExecContext(...)), check rowsAffected via result.RowsAffected(), and
return a clear error when rowsAffected == 0 (similar to how SetStripeCustomerID
handles non-existent organisations) so callers know the organisation wasn't
found.
In `@internal/db/users.go`:
- Around line 744-750: The code performs an extra DB query to fetch
stripe_customer_id using db.client.QueryRowContext and then sets
stats.HasStripeCustomer; instead, modify the get_organisation_usage_stats SQL
function to SELECT stripe_customer_id (or add it to its result set) and update
the caller to read that column into stats.HasStripeCustomer so you can remove
the separate QueryRowContext block and the sql.NullString handling.
In `@settings.html`:
- Around line 378-384: The Manage billing button (id="manageBillingBtn") lacks
an explicit type attribute and will default to type="submit", risking accidental
form submission; update the <button id="manageBillingBtn"> element to include
type="button" so it does not submit forms when clicked, keeping the id and
classes unchanged.
In `@supabase/migrations/20260406102929_add_stripe_billing.sql`:
- Around line 6-8: Add a UNIQUE constraint on organisations.stripe_customer_id
to enforce one-to-one mapping between organisations and Stripe customers: alter
the migration to create a unique constraint or unique index named (e.g.
organisations_stripe_customer_id_key) on stripe_customer_id, and ensure the
migration handles existing NULLs/duplicates (skip/clean duplicates or make
constraint DEFERRABLE/only for non-null values) so applying the migration won't
fail; reference the table name "organisations" and the column
"stripe_customer_id" when making the change.
In `@supabase/migrations/20260406112014_update_plans_to_new_tiers.sql`:
- Around line 76-86: The current UPDATEs (UPDATE organisations ... SET plan_id =
(SELECT id FROM plans WHERE name = 'pro') ...) can set plan_id to NULL if the
target plan rows don't exist; change each UPDATE to join against the plans table
so the update only runs when a matching target plan exists (e.g., use UPDATE
organisations o SET plan_id = p.id, updated_at = NOW() FROM plans p WHERE p.name
= 'pro' AND o.plan_id = (SELECT id FROM plans WHERE name = 'business')) and
apply the same pattern for the enterprise->ultra migration; optionally add a
guard that errors/logs if the target plan (p.id) is missing.
- Line 20: The migration currently hardcodes Stripe price IDs (e.g.,
stripe_price_id = 'price_1TJAjJS2RiCh0hZBfgrnoI0C') which ties it to a single
Stripe account/environment; instead, remove hardcoded IDs from the SQL and
replace the migration with a neutral schema-only change, then seed
stripe_price_id values from an environment-aware script or seed mechanism (read
IDs from env vars or a per-environment mapping file) that updates the
stripe_price_id column for Plan rows after deployment; also add a short comment
in the migration or repository README documenting which Stripe
account/environment the original IDs correspond to and reference the
stripe_price_id column and the specific price IDs previously used so maintainers
can map/test appropriately.
In `@web/static/app/lib/settings/plans.js`:
- Around line 137-149: The startCheckout function must prevent duplicate
submissions by disabling the Upgrade button while the async
post("/v1/billing/checkout", ...) is in progress: add an in-progress guard
(e.g., a local boolean or accept the button element as an optional parameter)
and, before calling post, set the guard and disable the button element (or set
disabled on elements with the Upgrade button selector), then perform the
request, and in a finally block re-enable the button and clear the guard; also
ensure the early return respects the guard so multiple clicks are ignored and
error handling (toast, console.error) remains unchanged.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c356f9ab-b233-492e-b116-9339505659b7
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (19)
.env.example.env.op.github/workflows/fly-deploy.yml.github/workflows/review-apps.yml.gitignorecmd/app/main.godev.shdocs/development/DEVELOPMENT.mdgo.modinternal/api/billing.gointernal/api/handlers.gointernal/api/stripe_webhook.gointernal/db/organisations.gointernal/db/users.gosettings.htmlsupabase/migrations/20260406102929_add_stripe_billing.sqlsupabase/migrations/20260406112014_update_plans_to_new_tiers.sqlweb/static/app/lib/settings/plans.jsweb/static/app/pages/settings.js
Contributor
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 @.github/workflows/fly-deploy.yml:
- Around line 60-66: The workflow file .github/workflows/fly-deploy.yml has
Prettier formatting failures around the Stripe secret key blocks
(STRIPE_SECRET_KEY, STRIPE_WEBHOOK_SECRET, STRIPE_PUBLISHABLE_KEY and the
similar block at lines referenced 83-85); run prettier --write
.github/workflows/fly-deploy.yml to reformat the YAML, verify the STRIPE_* env
entries remain properly indented and quoted if needed, then add/commit the
reformatted file so CI Prettier checks pass.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c40cc02d-93d8-4293-9ad3-ca4e1f3f33cd
📒 Files selected for processing (1)
.github/workflows/fly-deploy.yml
Contributor
|
🐝 Review App Deployed Homepage: https://hover-pr-301.fly.dev |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Contributor
|
🐝 Review App Deployed Homepage: https://hover-pr-301.fly.dev |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/api/billing.go`:
- Around line 69-99: Add an info-level log right after successfully creating the
Stripe customer in the billing flow: after customer.New returns (where cust and
customerID are set) log something like "Created Stripe customer" including
cust.ID (customerID), orgID, user.Email and org.Name for auditability; place
this log immediately before calling h.DB.SetStripeCustomerID so both the
creation and the subsequent DB write are clearly traceable (refer to
customer.New, cust.ID/customerID, orgID, user.Email, org.Name, and
h.DB.SetStripeCustomerID).
In `@internal/api/stripe_webhook.go`:
- Around line 146-156: The subscription update handler accesses
sub.Items.Data[0].Price.ID without a nil guard; add the same nil check used in
the checkout handler: verify sub.Items.Data[0].Price != nil before reading ID,
and if nil log a warning (include org_id and a message like
"subscription.updated: no price on line item — skipping plan update") then
return; this change touches the block that assigns priceID and calls
h.DB.GetPlanByStripePriceID so add the guard immediately above where priceID is
set to avoid a nil dereference.
- Around line 109-119: Add a nil guard before dereferencing the Price on the
first subscription item: check that sub.Items.Data[0].Price is not nil before
accessing .ID, and if it is nil log an error (include subscription id/context)
via logger.Error() and return, mirroring existing customer nil-guard style;
update the block around where priceID is set and the call to
h.DB.GetPlanByStripePriceID to only run when Price is non-nil so you avoid a
potential panic.
In `@web/static/app/lib/settings/plans.js`:
- Around line 165-206: loadBillingSection currently issues its own
get("/v1/usage") call causing duplicate requests when loadPlansAndUsage also
fetches usage; either modify loadBillingSection to accept an optional
usageResponse parameter (e.g., loadBillingSection(usageResponse)) and skip the
internal fetch when provided, or implement a module-scoped cache (e.g.,
cachedUsage + cachedUsageTs) checked before calling get("/v1/usage") with a
short TTL (e.g., 30s) and populate the cache after a successful fetch; update
callers (loadPlansAndUsage or settings init) to pass the already-fetched usage
or rely on the cache so only one network request is made.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 15d7627b-434a-4a9f-9a6d-14b2a741de3c
📒 Files selected for processing (5)
internal/api/billing.gointernal/api/handlers.gointernal/api/stripe_webhook.gosettings.htmlweb/static/app/lib/settings/plans.js
Contributor
Author
StatusAll CodeRabbit review comments resolved. CI passing (Format, Lint, Supabase Preview, Changelog). Deploy Review App and Integration/Unit Tests running. What's in this PR
To test on review app
To finalise before merge
|
Contributor
|
🐝 Review App Deployed Homepage: https://hover-pr-301.fly.dev |
2 similar comments
Contributor
|
🐝 Review App Deployed Homepage: https://hover-pr-301.fly.dev |
Contributor
|
🐝 Review App Deployed Homepage: https://hover-pr-301.fly.dev |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/static/app/lib/settings/plans.js (1)
123-128:⚠️ Potential issue | 🔴 CriticalCancel Stripe subscription when downgrading to Free, or route through Billing Portal.
The direct free-plan switch via
PUT /v1/organisations/plan(lines 141–149) updates the local database without cancelling the Stripe subscription. Meanwhile,internal/api/stripe_webhook.golines 175–203 treatcustomer.subscription.deletedas the source of truth for reverting to Free. This creates a window where the app marks the organisation free whilst Stripe continues to charge. The downgrade should either:
- Trigger Stripe subscription cancellation as part of the plan update, or
- Route through the Billing Portal (like upgrades do) and let the webhook apply the downgrade.
Also,
getUsage()caches for 30 seconds; whenswitchPlan()reloads the plans UI within that window, stale plan data may render briefly. Clear the cache after a successful plan mutation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/static/app/lib/settings/plans.js` around lines 123 - 128, The current direct free-plan switch via switchPlan(plan.id, ...) updates your DB but doesn't cancel the Stripe subscription; update switchPlan (and the client-side flow tied to the "Switch to Free" button) to 1) either call an API endpoint that cancels the Stripe subscription as part of the plan change (ensure server-side handler for PUT /v1/organisations/plan performs Stripe subscription cancellation) or 2) route the user through the Billing Portal flow used by upgrades and let the existing customer.subscription.deleted webhook handle the downgrade; additionally, after a successful plan mutation clear the getUsage() cache (invalidate its 30s cache) before reloading the plans UI so stale plan data doesn't render.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/api/billing.go`:
- Around line 37-40: The current check conflates DB errors from
GetOrganisationMemberRole with authorization failures and always returns
Forbidden; change the logic to first handle an error from
GetOrganisationMemberRole by logging it and returning a 5xx
(InternalServerError) response, and only return Forbidden when err == nil but
role != "admin". Update both occurrences that call GetOrganisationMemberRole
(the one around role, err := h.DB.GetOrganisationMemberRole(...) and the second
occurrence at lines ~149-152) to follow this pattern and use the existing
logging and response helpers (e.g., log the error and call the appropriate
InternalServerError helper) so backend faults aren’t masked as permission
denials.
- Around line 93-95: The audit log at the Stripe customer creation only needs
non-sensitive identifiers: remove user.Email and org.Name from the log call and
keep orgID and customerID; update the log invocation that uses
log.Info().Str(...).Msg("Created Stripe customer") so it only attaches
"customer_id" and "org_id" (and any safe metadata), and leave the DB call to
h.DB.SetStripeCustomerID(r.Context(), orgID, customerID) unchanged; ensure no
other user-identifying fields (e.g., user.Email, org.Name) are added elsewhere
in the surrounding function.
- Around line 69-118: The handler creates a Checkout Session in subscription
mode without checking for existing active subscriptions, which can create
duplicate live subscriptions; before creating the session (before calling
checkoutsession.New) query the organisation's active subscription status via the
DB (e.g., call a repository method such as h.DB.HasActiveSubscription(orgID) or
h.DB.GetActiveSubscription(orgID)), and if an active subscription exists return
a rejection (400/409) or redirect/point the caller to the BillingPortal handler
instead of proceeding; integrate this check near the customer lookup/creation
logic (around GetStripeCustomerID/GetOrganisation/SetStripeCustomerID) and bail
out early when an active subscription is found.
In `@internal/api/stripe_webhook.go`:
- Around line 61-62: The webhook currently unconditionally writes http.StatusOK;
change the handler in internal/api/stripe_webhook.go so that it returns and
writes 5xx on transient failures from the event processing paths (specifically
where you persist Stripe IDs and apply plan updates) instead of always
acknowledging; propagate errors from the functions that perform the Stripe ID
writes and the plan update logic so the outer handler can detect failures, log
them, and respond with an appropriate 5xx HTTP status for transient DB/Stripe
API errors, and only write 200 OK when the event was successfully applied or
intentionally ignored.
In `@web/static/app/lib/settings/plans.js`:
- Around line 17-29: The cached usage in getUsage() (_cachedUsage and
_cachedUsageTs) must be invalidated immediately after a successful plan change
so loadPlansAndUsage() doesn't read stale data; update the switchPlan() (the
code that does the PUT /v1/organisations/plan) to clear _cachedUsage = null and
_cachedUsageTs = 0 (or otherwise force a fresh fetch) on successful response (or
trigger a fresh getUsage() call) so the UI shows the updated plan/limits right
away.
---
Outside diff comments:
In `@web/static/app/lib/settings/plans.js`:
- Around line 123-128: The current direct free-plan switch via
switchPlan(plan.id, ...) updates your DB but doesn't cancel the Stripe
subscription; update switchPlan (and the client-side flow tied to the "Switch to
Free" button) to 1) either call an API endpoint that cancels the Stripe
subscription as part of the plan change (ensure server-side handler for PUT
/v1/organisations/plan performs Stripe subscription cancellation) or 2) route
the user through the Billing Portal flow used by upgrades and let the existing
customer.subscription.deleted webhook handle the downgrade; additionally, after
a successful plan mutation clear the getUsage() cache (invalidate its 30s cache)
before reloading the plans UI so stale plan data doesn't render.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5b7ae426-a05a-4011-ad8d-965e1124517a
📒 Files selected for processing (4)
go.modinternal/api/billing.gointernal/api/stripe_webhook.goweb/static/app/lib/settings/plans.js
Contributor
|
🐝 Review App Deployed Homepage: https://hover-pr-301.fly.dev |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
simonsmallchua
force-pushed
the
worktree-calm-hugging-pebble
branch
from
93074ce to
1c97a4f
Compare
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Contributor
|
🐝 Review App Deployed Homepage: https://hover-pr-301.fly.dev |
Contributor
|
🐝 Review App Deployed Homepage: https://hover-pr-301.fly.dev |
Contributor
|
🐝 Review App Deployed Homepage: https://hover-pr-301.fly.dev |
Contributor
|
🐝 Review App Deployed Homepage: https://hover-pr-301.fly.dev |
Contributor
|
🐝 Review App Deployed Homepage: https://hover-pr-301.fly.dev |
Contributor
|
🐝 Review App Deployed Homepage: https://hover-pr-301.fly.dev |
Contributor
There was a problem hiding this comment.
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)
web/static/app/pages/settings.js (1)
264-276:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear the shared usage cache before an organisation refresh.
refreshSections()is reused bygnh:org-switched, but bothloadPlansAndUsage()andloadBillingSection()read the global usage cache fromweb/static/app/lib/settings/plans.js. Without aninvalidateUsageCache()here, switching organisations inside the cache TTL can show the previous organisation’s plan and billing state.Suggested patch
async function refreshSections() { + invalidateUsageCache(); const c = getContainers(); const teamState = getTeamState(); try {Also applies to: 289-290
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/static/app/pages/settings.js` around lines 264 - 276, refreshSections() must clear the shared usage cache before loading plan/billing data: call the cache invalidator (e.g. invalidateUsageCache()) before invoking loadPlansAndUsage() and loadBillingSection() inside refreshSections() so the calls read fresh organisation-specific data; also ensure the same invalidateUsageCache() is invoked from the org switch path (gnh:org-switched) or any other place that reuses refreshSections so stale cached usage cannot leak between orgs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/actions/fly-setup/action.yml:
- Around line 80-86: The workflow currently only validates REDIS_URL but must
fail fast if any Stripe secret resolves to an empty string; update the
validation logic in the action.yml step that checks secrets to also verify
STRIPE_SECRET_KEY, STRIPE_WEBHOOK_SECRET, and STRIPE_PUBLISHABLE_KEY (and the
duplicate set of vars present on lines ~90-97) are non-empty, and if any are
empty exit with a non-zero status and a clear error logged (e.g., “Missing
required secret: STRIPE_SECRET_KEY”) so flyctl secrets set is never called with
blank values.
In `@internal/api/billing.go`:
- Around line 177-217: The Checkout Session creation path currently calls
checkoutsession.New(...) with a constructed *stripe.CheckoutSessionParams but
lacks an idempotency key; modify the block that builds CheckoutSessionParams
(the variable passed to checkoutsession.New) to create the params separately,
call params.SetIdempotencyKey(...) with a deterministic key (e.g.
fmt.Sprintf("%s-%s", orgID, stripePriceID)), then pass that params to
checkoutsession.New; this uses the existing symbols CheckoutSessionParams,
SetIdempotencyKey, checkoutsession.New, orgID and stripePriceID to prevent
duplicate sessions on retries.
In `@internal/api/stripe_webhook.go`:
- Around line 96-99: The DB lookup errors from h.DB helpers (e.g.,
GetOrganisationIDByStripeCustomerID, the plan/price lookup helpers referenced)
must expose a sentinel "not found" error instead of being wrapped into generic
errors; update the DB functions to return a specific exported error value (e.g.,
ErrOrganisationNotFound, ErrPlanNotFound) and preserve other errors as
transient. In stripe_webhook.go, detect these sentinel errors after calls like
h.DB.GetOrganisationIDByStripeCustomerID, log the missing-resource with
logger.Error().Str(...).Msg(...), and then ACK the webhook (return nil) so
Stripe won't retry; keep existing 5xx behavior (returning the wrapped error) for
non-sentinel errors. Apply the same sentinel-check-and-ack pattern to the other
webhook handler sites noted (the lookups at the other referenced locations).
- Around line 168-195: Resolve and compare the organisation's stored Stripe
subscription ID before mutating plan state: after obtaining orgID with
h.DB.GetOrganisationIDByStripeCustomerID and before using sub.ID to update the
plan, fetch the organisation's stored stripe_subscription_id (via the existing
DB method that returns org details or a new
GetOrganisationByID/GetStripeSubscriptionID function) and if the stored ID is
non-empty and does not equal sub.ID, log and return without changing the plan;
only proceed to call h.DB.SetOrganisationPlan when the IDs match (apply the same
guard to the other handler referenced around lines 211-233).
In `@internal/db/users.go`:
- Around line 691-697: The Stripe-customer lookup is swallowing database errors
and forcing stats.HasStripeCustomer to false; change the logic around
db.client.QueryRowContext so that when Scan returns an error (other than
sql.ErrNoRows if you explicitly want to treat missing row as no customer) you
return or propagate that error instead of silently setting
stats.HasStripeCustomer to false; locate the QueryRowContext/Scan call and the
stats.HasStripeCustomer assignment and ensure any non-nil scan error is returned
(or wrapped) to the caller so real DB faults are not masked.
In `@web/static/app/pages/settings.js`:
- Around line 241-259: handleBillingRedirect currently treats billing=success as
final; change it to wait for the webhook-driven state before showing success.
When billing === "success", show a temporary "Finalising subscription…"
UI/toast, then poll the usage/org endpoint (e.g., call the same fetch used by
invalidateUsageCache or the /v1/usage getter) on an interval with a short
timeout; only when the returned usage/plan reflects the new subscription call
invalidateUsageCache() and show the success toast, otherwise on timeout show an
error/warning. Keep billing deletion and history.replaceState behavior the same,
and reference handleBillingRedirect, invalidateUsageCache, and the existing
usage fetch function when implementing the poll-and-validate flow.
---
Outside diff comments:
In `@web/static/app/pages/settings.js`:
- Around line 264-276: refreshSections() must clear the shared usage cache
before loading plan/billing data: call the cache invalidator (e.g.
invalidateUsageCache()) before invoking loadPlansAndUsage() and
loadBillingSection() inside refreshSections() so the calls read fresh
organisation-specific data; also ensure the same invalidateUsageCache() is
invoked from the org switch path (gnh:org-switched) or any other place that
reuses refreshSections so stale cached usage cannot leak between orgs.
🪄 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: CHILL
Plan: Pro
Run ID: 6c5cb00e-c895-4669-a123-338f6a1af3cd
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (25)
.env.example.env.op.fly/review_apps.toml.github/actions/fly-setup/action.yml.github/workflows/fly-deploy.yml.github/workflows/review-apps.yml.gitignoreCHANGELOG.mdcmd/app/main.godev.shdocs/development/DEVELOPMENT.mdfly.tomlgo.modinternal/api/billing.gointernal/api/errors.gointernal/api/handlers.gointernal/api/stripe_webhook.gointernal/db/organisations.gointernal/db/users.gosettings.htmlsupabase/migrations/20260428210000_add_stripe_billing.sqlsupabase/migrations/20260428210001_update_plans_to_new_tiers.sqlsupabase/seed.sqlweb/static/app/lib/settings/plans.jsweb/static/app/pages/settings.js
✅ Files skipped from review due to trivial changes (9)
- internal/api/errors.go
- .fly/review_apps.toml
- docs/development/DEVELOPMENT.md
- fly.toml
- .gitignore
- go.mod
- supabase/migrations/20260428210000_add_stripe_billing.sql
- CHANGELOG.md
- internal/db/organisations.go
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/review-apps.yml
- web/static/app/lib/settings/plans.js
Comment on lines
+241
to
+259
| function handleBillingRedirect() { | ||
| const params = new URLSearchParams(window.location.search); | ||
| const billing = params.get("billing"); | ||
| if (!billing) return; | ||
|
|
||
| if (billing === "success") { | ||
| invalidateUsageCache(); | ||
| toast("success", "Subscription activated"); | ||
| } else if (billing === "cancelled") { | ||
| toast("warning", "Checkout cancelled"); | ||
| } | ||
|
|
||
| params.delete("billing"); | ||
| const cleanQuery = params.toString(); | ||
| const cleanUrl = | ||
| window.location.pathname + | ||
| (cleanQuery ? "?" + cleanQuery : "") + | ||
| window.location.hash; | ||
| window.history.replaceState({}, "", cleanUrl); |
Contributor
There was a problem hiding this comment.
Wait for webhook-driven billing state before showing success.
billing=success only means Checkout redirected back; the actual plan change is applied by the webhook. Right now this path invalidates once and immediately shows “Subscription activated”, so if /v1/usage is fetched before checkout.session.completed updates the organisation, the page can still render the old plan and disabled billing state on return. Poll until usage reflects the new subscription, or show a “Finalising subscription…” state until it does.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/static/app/pages/settings.js` around lines 241 - 259,
handleBillingRedirect currently treats billing=success as final; change it to
wait for the webhook-driven state before showing success. When billing ===
"success", show a temporary "Finalising subscription…" UI/toast, then poll the
usage/org endpoint (e.g., call the same fetch used by invalidateUsageCache or
the /v1/usage getter) on an interval with a short timeout; only when the
returned usage/plan reflects the new subscription call invalidateUsageCache()
and show the success toast, otherwise on timeout show an error/warning. Keep
billing deletion and history.replaceState behavior the same, and reference
handleBillingRedirect, invalidateUsageCache, and the existing usage fetch
function when implementing the poll-and-validate flow.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
web/static/app/pages/settings.js (1)
246-249:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t treat
billing=successas final subscription state.
checkoutredirect success only confirms return from Stripe; plan activation is webhook-driven. Showing “Subscription activated” immediately can still present stale plan/billing UI when webhook processing lags.Please gate the success toast behind a short poll/confirmation of updated usage/plan state (or show a “Finalising subscription…” interim state until confirmed).
internal/api/stripe_webhook.go (2)
143-147:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftSeparate permanent “not found” misses from transient webhook failures.
These lookup errors are still escalated as transient 5xx, but unknown customer/price/free-plan mapping can be permanent data/config misses. That causes indefinite Stripe retries for events that won’t ever succeed.
Introduce sentinel DB errors for not-found cases and ACK those paths after logging; keep 5xx only for genuine transient DB/Stripe failures.
Also applies to: 168-172, 198-202, 224-228, 242-246
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/stripe_webhook.go` around lines 143 - 147, The DB lookup failures for permanent "not found" cases (e.g., the call to h.DB.GetPlanByStripePriceID) should be distinguished from transient errors: detect the sentinel not-found error returned by your DB layer (e.g., ErrNotFound or sql.ErrNoRows) when resolving plans/customers/mappings, log an error with context (price_id/customer_id/etc.), and ACK the webhook by returning nil instead of propagating a 5xx error; only return an error for genuine transient DB/Stripe failures so Stripe will retry. Apply the same change to the other lookup sites noted (the similar Get... calls around the other ranges) so missing mappings are logged and acknowledged rather than retried.
178-185:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPersist adopted subscription ID when local state is empty.
The “non-current subscription” guard only fully works if an empty stored ID is adopted. Right now, when stored ID is empty, updates/deletes proceed without persisting
sub.ID, so future stale events can still mutate plan state.Suggested patch
storedSubID, err := h.DB.GetStripeSubscriptionID(r.Context(), orgID) if err != nil { return fmt.Errorf("fetch stored subscription id: %w", err) } + if storedSubID == "" && sub.ID != "" { + if err := h.DB.SetStripeSubscriptionID(r.Context(), orgID, sub.ID); err != nil { + return fmt.Errorf("adopt stripe subscription id: %w", err) + } + storedSubID = sub.ID + } if storedSubID != "" && storedSubID != sub.ID { logger.Warn().Str("org_id", orgID).Str("event_subscription_id", sub.ID).Str("stored_subscription_id", storedSubID).Msg("Ignoring subscription.updated for non-current subscription") return nil }Apply the same adoption/empty-state handling strategy in
handleSubscriptionDeleted.Also applies to: 233-240
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/stripe_webhook.go` around lines 178 - 185, The "non-current subscription" guard needs to adopt an empty local subscription id so future stale events don't mutate state: in handleSubscriptionDeleted (and the similar block at the other handler around the same area) after calling h.DB.GetStripeSubscriptionID(ctx, orgID) and seeing storedSubID == "", persist the incoming sub.ID by calling the DB setter (e.g. h.DB.SetStripeSubscriptionID(ctx, orgID, sub.ID) or the existing project equivalent) before returning/ignoring the event; leave the existing warning/ignore behavior when storedSubID is non-empty and different from sub.ID otherwise.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/api/stripe_webhook.go`:
- Around line 143-147: The DB lookup failures for permanent "not found" cases
(e.g., the call to h.DB.GetPlanByStripePriceID) should be distinguished from
transient errors: detect the sentinel not-found error returned by your DB layer
(e.g., ErrNotFound or sql.ErrNoRows) when resolving plans/customers/mappings,
log an error with context (price_id/customer_id/etc.), and ACK the webhook by
returning nil instead of propagating a 5xx error; only return an error for
genuine transient DB/Stripe failures so Stripe will retry. Apply the same change
to the other lookup sites noted (the similar Get... calls around the other
ranges) so missing mappings are logged and acknowledged rather than retried.
- Around line 178-185: The "non-current subscription" guard needs to adopt an
empty local subscription id so future stale events don't mutate state: in
handleSubscriptionDeleted (and the similar block at the other handler around the
same area) after calling h.DB.GetStripeSubscriptionID(ctx, orgID) and seeing
storedSubID == "", persist the incoming sub.ID by calling the DB setter (e.g.
h.DB.SetStripeSubscriptionID(ctx, orgID, sub.ID) or the existing project
equivalent) before returning/ignoring the event; leave the existing
warning/ignore behavior when storedSubID is non-empty and different from sub.ID
otherwise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b1a9fbd3-095c-4738-9673-468407fd1008
📒 Files selected for processing (5)
.github/actions/fly-setup/action.ymlinternal/api/billing.gointernal/api/stripe_webhook.gointernal/db/users.goweb/static/app/pages/settings.js
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/api/billing.go
Contributor
|
🐝 Review App Deployed Homepage: https://hover-pr-301.fly.dev |
Contributor
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 `@internal/api/stripe_webhook.go`:
- Around line 193-203: The current logic in the webhook handler (using
h.DB.GetStripeSubscriptionID, h.DB.SetStripeSubscriptionID and variables
storedSubID/sub.ID) incorrectly adopts subscription IDs from arbitrary
subscription.update/delete events; change this so you only seed or replace an
empty stripe_subscription_id when the event is an authoritative
checkout.session.completed or after validating Stripe's current subscription
state via a lookup (e.g., call Stripe to fetch the subscription by sub.ID and
confirm it's active/current) before calling h.DB.SetStripeSubscriptionID; apply
the same pattern to the other handler block at the 264-273 region to avoid
accepting stale events.
🪄 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: CHILL
Plan: Pro
Run ID: fa104e80-cdf9-4df6-afc2-d0d8d2b6a4f2
📒 Files selected for processing (3)
internal/api/stripe_webhook.gointernal/db/db.gointernal/db/organisations.go
✅ Files skipped from review due to trivial changes (1)
- internal/db/organisations.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/api/stripe_webhook.go (1)
130-136: 💤 Low valueConsider propagating request context to the Stripe API call.
The
stripesubscription.Getcall doesn't receive the request context, so it won't respect request cancellation or timeouts. The stripe-go library supports context via params:- sub, err := stripesubscription.Get(subID, nil) + params := &stripe.SubscriptionParams{} + params.Context = r.Context() + sub, err := stripesubscription.Get(subID, params)This ensures the call respects any configured server timeouts and allows graceful cancellation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/stripe_webhook.go` around lines 130 - 136, The Stripe subscription fetch currently calls stripesubscription.Get without a request context; update the call to pass the request context (e.g., ctx) via the stripe params so the call respects cancellation/timeouts — for example, construct params that include stripe.Params{Context: ctx} (or the library-equivalent) and pass that into stripesubscription.Get (referencing stripesubscription.Get and the params struct used) so the request is cancellable and bound to the incoming request's deadline.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/api/stripe_webhook.go`:
- Around line 130-136: The Stripe subscription fetch currently calls
stripesubscription.Get without a request context; update the call to pass the
request context (e.g., ctx) via the stripe params so the call respects
cancellation/timeouts — for example, construct params that include
stripe.Params{Context: ctx} (or the library-equivalent) and pass that into
stripesubscription.Get (referencing stripesubscription.Get and the params struct
used) so the request is cancellable and bound to the incoming request's
deadline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 39ac2262-b41b-4c5a-94b1-015d4a977201
📒 Files selected for processing (1)
internal/api/stripe_webhook.go
Contributor
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 `@internal/api/stripe_webhook.go`:
- Around line 235-250: Before using the webhook payload's subscription data to
pick a plan, re-fetch the authoritative subscription from Stripe (similar to the
BillingCheckout pattern) to avoid applying stale out-of-order events: call the
Stripe API to retrieve the subscription for sub.ID (or use
h.StripeClient.Subscriptions.Get/Equivalent) and then read the latest priceID
from the returned subscription before calling h.DB.GetPlanByStripePriceID and
h.DB.SetOrganisationPlan; update the code paths around
sub.Items.Data[0].Price.ID, plan resolution, and SetOrganisationPlan to use the
refreshed subscription object.
🪄 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: CHILL
Plan: Pro
Run ID: 22ba7ab9-434b-4e8b-aea7-f5c08e8d87ac
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
go.modinternal/api/billing.gointernal/api/stripe_webhook.go
✅ Files skipped from review due to trivial changes (2)
- go.mod
- internal/api/billing.go
Contributor
|
🐝 Review App Deployed Homepage: https://hover-pr-301.fly.dev |
Contributor
|
🐝 Review App Deployed Homepage: https://hover-pr-301.fly.dev |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
op injecttodev.shso local dev secrets are pulled from 1Password automaticallyChanges
supabase/migrations/— addsstripe_price_idto plans,stripe_customer_id/stripe_subscription_idto organisations; updates plan tiers with real Stripe Price IDsinternal/api/billing.go—POST /v1/billing/checkoutandPOST /v1/billing/portalendpointsinternal/api/stripe_webhook.go— handlescheckout.session.completed,subscription.updated,subscription.deleted,invoice.payment_failedinternal/db/— 6 new Stripe-related DB helpersweb/static/app/— settings page billing section: Upgrade buttons, portal link, plan display.github/workflows/— Stripe keys injected from 1Password for review apps and production.env.op— committedop://template for local dev secrets (no actual secrets)dev.sh— auto-injects secrets from 1Password on startupTest plan
./dev.sh— confirms 1Password secrets load (or warns gracefully ifopnot signed in)/dev/auto-login→ Settings → Plans — Upgrade buttons visible for paid tiersstripe listen --forward-to localhost:8080/webhooks/stripe→ webhook events handled correctlyCloses #294
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
Database