Billing SDK Refactor #2722
Billing SDK Refactor #2722
Conversation
replace: `billing.listOrganization` calls with `organizations.list`.
…tionDeleteOrganization`, `getOrganizationPlan` and `getRoles`.
Console (appwrite/console)Project ID: Tip Storage files get ClamAV malware scanning and encryption by default |
|
Warning Rate limit exceeded@ItzNotABug has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 2 minutes and 46 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the 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. Please see our FAQ for further information. 📒 Files selected for processing (8)
WalkthroughThis pull request refactors the billing and organization infrastructure across the application. Key changes include: (1) migrating from local, custom type definitions to centralized Models types from Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
…od and its usages.
…hods and its usages.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
src/routes/(console)/organization-[organization]/billing/availableCredit.svelte (1)
36-51: Add error handling to the API request.The
request()function lacks error handling. If the API call fails, the UI remains in an inconsistent state with no feedback to the user. This is inconsistent with similar components in the billing directory, which all wrap SDK calls in try-catch blocks.🔎 Proposed fix to add error handling
async function request() { if (!$organization?.$id) return; // fast path return! if (!areCreditsSupported) return; /** * The initial `creditList` from `+page.ts` can include up to 25 items by default. * * Technically, we could reuse that for offsets < 25 (i.e., the first 5 pages with limit = 5) * to avoid an extra request. But for now, we always fetch fresh data. */ - creditList = await sdk.forConsole.organizations.listCredits({ - organizationId: $organization.$id, - queries: [Query.limit(limit), Query.offset(offset)] - }); + try { + creditList = await sdk.forConsole.organizations.listCredits({ + organizationId: $organization.$id, + queries: [Query.limit(limit), Query.offset(offset)] + }); + } catch (error) { + console.error('Failed to load credits:', error); + // Consider showing user feedback (toast/alert) here + return; + } creditList = {src/lib/components/billing/selectPaymentMethod.svelte (1)
25-29: Avoid manually constructing SDK model objects.Manually spreading and reconstructing
Models.PaymentMethodListviolates the principle that SDK models should be treated as immutable data from the API. This pattern can break if the SDK type has readonly properties, class methods, or internal state.Consider refetching the payment methods list after adding a new card, or check if the SDK provides a method to update the list. The existing invalidate calls on lines 32-35 suggest this data will be refreshed anyway.
💡 Suggested refactor
Since you're already invalidating dependencies, consider removing the manual update and relying on the data refetch:
async function cardSaved(card: Models.PaymentMethod) { value = card.$id; - if (value) { - methods = { - ...methods, - total: methods.total + 1, - paymentMethods: [...methods.paymentMethods, card] - }; - } - await Promise.all([ invalidate(Dependencies.UPGRADE_PLAN), invalidate(Dependencies.ORGANIZATION) ]); }If immediate UI update is required before refetch, verify that the SDK models support this mutation pattern or use a local state approach that doesn't mutate the SDK model.
src/lib/stores/billing.ts (1)
534-552: Useorganizations.getPaymentMethodinstead ofaccount.getPaymentMethod.
checkForMandateretrieves an organization's payment method but usessdk.forConsole.account.getPaymentMethodwithout passing the organization ID. This differs frompaymentExpired(line 470) which usessdk.forConsole.organizations.getPaymentMethodwithorganizationId. For consistency and to ensure proper organization-level access control, use the same endpoint pattern aspaymentExpired:const paymentMethod = await sdk.forConsole.organizations.getPaymentMethod({ organizationId: org.$id, paymentMethodId: paymentId });src/routes/(console)/create-organization/+page.ts (1)
45-57: Missingawaitin getCoupon causes the function to return a Promise instead of resolving it.The
try/catchblock won't catch any errors from the SDK call because the promise is returned directly without awaiting. This means errors will propagate as unhandled promise rejections rather than being caught and returningnull.🔎 Proposed fix
async function getCoupon(url: URL): Promise<Models.Coupon | null> { if (url.searchParams.has('code')) { const coupon = url.searchParams.get('code'); try { - return sdk.forConsole.account.getCoupon({ + return await sdk.forConsole.account.getCoupon({ couponId: coupon }); } catch (e) { return null; } } return null; }src/routes/(console)/organization-[organization]/billing/wizard/addCredit.svelte (1)
26-31: Incorrect error wrapping loses error details.
throw new Error(error)will callerror.toString()which typically results in[object Object]. This loses the original error's stack trace and structured information.🔎 Proposed fix
} catch (error) { couponData.code = coupon; couponData.status = 'error'; $wizard.interceptorNotificationEnabled = false; - throw new Error(error); + throw error; }src/routes/(public)/sites/deploy/+page.ts (1)
72-77: Inconsistent API call syntax - uses positional arguments instead of object syntax.This file uses positional arguments for
organizations.create:await sdk.forConsole.organizations.create( ID.unique(), 'Personal Projects', BillingPlan.FREE, null );Other files in this PR (e.g.,
src/routes/(public)/template-[template]/+page.tslines 43-47 andsrc/routes/(public)/sites/deploy/+page.tslines 88-92) use object syntax:await sdk.forConsole.organizations.create({ organizationId: ID.unique(), name: 'Personal Projects', billingPlan: BillingPlan.FREE });This positional syntax may be incorrect or deprecated. Please verify and align with the object-based syntax used elsewhere.
🔎 Suggested fix using object syntax
if (isCloud) { - await sdk.forConsole.organizations.create( - ID.unique(), - 'Personal Projects', - BillingPlan.FREE, - null - ); + await sdk.forConsole.organizations.create({ + organizationId: ID.unique(), + name: 'Personal Projects', + billingPlan: BillingPlan.FREE + }); } else {src/routes/(public)/functions/deploy/+page.ts (1)
67-85: Same API syntax inconsistency as sites/deploy.This file has the identical issue with positional arguments for
organizations.create(lines 72-77) that was flagged insrc/routes/(public)/sites/deploy/+page.ts. Please apply the same fix to use object syntax.🔎 Suggested fix using object syntax
if (isCloud) { - await sdk.forConsole.organizations.create( - ID.unique(), - 'Personal Projects', - BillingPlan.FREE, - null - ); + await sdk.forConsole.organizations.create({ + organizationId: ID.unique(), + name: 'Personal Projects', + billingPlan: BillingPlan.FREE + }); } else {
🧹 Nitpick comments (19)
src/lib/flags.ts (1)
8-20: Consider removing unused code or clarifying its purpose.The
isFlagEnabledfunction is marked as unused and never called or exported. The file also exports an emptyflagsobject (line 22), suggesting incomplete or placeholder implementation. Consider either:
- Removing the unused function to reduce clutter
- Adding a comment explaining why this code is being retained for future use
src/routes/(console)/organization-[organization]/billing/taxId.svelte (1)
20-23: LGTM! SDK refactor is correct.The migration from
sdk.forConsole.billing.updateTaxIdtosdk.forConsole.organizations.setBillingTaxIdwith named parameters is well-executed and aligns with the PR objectives.Optional: Consider adding null-safety for consistency.
Lines 15 and 49 use optional chaining on
$organization, but line 21 directly accesses$organization.$id. While the route likely ensures the organization is loaded, adding optional chaining or an early return would improve consistency and defensive coding:🔎 Optional refactor for null-safety
async function updateTaxId() { + if (!$organization) return; + try { await sdk.forConsole.organizations.setBillingTaxId({ organizationId: $organization.$id, taxId });src/routes/(public)/(guest)/login/+page.ts (1)
8-27: API migration looks good; consider explicit typing for consistency.The migration to
console.getCouponandconsole.getCampaignwith object-based parameters is correct and matches the patterns inregister/+page.tsandapply-credit/+page.ts.For consistency with similar files in this refactor (e.g.,
register/+page.tslines 7-8,apply-credit/+page.tslines 6-7), consider explicitly typingcouponDataasModels.Coupon.🔎 Optional: Add explicit type for consistency
if (url.searchParams.has('code')) { const code = url.searchParams.get('code'); let campaign: Models.Campaign; + let couponData: Models.Coupon; try { - const couponData = await sdk.forConsole.console.getCoupon({ + couponData = await sdk.forConsole.console.getCoupon({ couponId: code });src/lib/components/billing/alerts/paymentMandate.svelte (1)
10-15: Consider adding error handling for payment operations.Payment verification failures should provide clear feedback to users. Wrapping the async operations in try-catch would improve the user experience.
🔎 Suggested error handling
async function verifyPaymentMethod() { + try { const method = await sdk.forConsole.account.updatePaymentMethodMandateOptions({ paymentMethodId: $paymentMissingMandate.$id }); await confirmSetup(method.clientSecret, method.providerMethodId); + } catch (error) { + // Handle error appropriately (e.g., show toast notification) + console.error('Payment verification failed:', error); + } }src/lib/components/billing/alerts/selectProjectCloud.svelte (1)
45-47: Consider typing the caught error for type safety.The caught exception
eis not typed, and accessinge.messagedirectly is not type-safe. Consider typing it explicitly or using a type guard.🔎 Suggested improvement
-} catch (e) { - error = e.message; +} catch (e) { + error = e instanceof Error ? e.message : String(e); }src/routes/(console)/account/payments/editPaymentModal.svelte (1)
30-32: Consider awaitinginvalidatebefore showing the notification.The
invalidate(Dependencies.PAYMENT_METHODS)call is not awaited. If the user performs another action immediately after closing the modal, the payment methods list might not yet be refreshed, leading to stale data being displayed.🔎 Proposed fix
trackEvent(Submit.PaymentMethodUpdate); - invalidate(Dependencies.PAYMENT_METHODS); + await invalidate(Dependencies.PAYMENT_METHODS); show = false;src/lib/components/billing/emptyCardCloud.svelte (1)
20-20:proPlanNameis not reactive to store changes.
proPlanNameis assigned once when the component initializes. IfplansInfostore updates after mount (e.g., due to data refresh), this value won't update. Consider using$derivedif reactivity is needed:🔎 Proposed fix using $derived
- const proPlanName = getBasePlanFromGroup(BillingPlanGroup.Pro).name; + const proPlanName = $derived(getBasePlanFromGroup(BillingPlanGroup.Pro).name);However, if plan names are stable after initial load, the current implementation is acceptable.
src/routes/(console)/organization-[organization]/billing/deleteOrgPayment.svelte (1)
18-37: Inconsistentawaitusage forinvalidate().In
removeDefaultMethod(line 30),invalidate()is called withoutawait, while inremoveBackupMethod(line 53), it's awaited. This inconsistency could lead to different behaviors—the backup method waits for cache invalidation before proceeding, while the default method does not.🔎 Proposed fix for consistency
trackEvent(Submit.OrganizationPaymentDelete); - invalidate(Dependencies.ORGANIZATION); + await invalidate(Dependencies.ORGANIZATION); showDelete = false;Also applies to: 40-59
src/routes/(console)/organization-[organization]/domains/domain-[domain]/settings/+page.ts (1)
8-16: Add type annotation for the load function.The
loadfunction is missing itsPageLoadtype annotation which other route loaders in the codebase use for type safety.Suggested fix
+import type { PageLoad } from './$types'; -export const load = async ({ params, parent, depends }) => { +export const load: PageLoad = async ({ params, parent, depends }) => {src/routes/(console)/create-organization/+page.ts (1)
35-43: Simplify redundant conditional check.The
if (plan)check is redundant sinceurl.searchParams.get('plan')returnsnullif the param doesn't exist, and the outerifalready confirms the param exists.🔎 Proposed simplification
-function getPlanFromUrl(url: URL): string | null { - if (url.searchParams.has('plan')) { - const plan = url.searchParams.get('plan'); - if (plan) { - return plan; - } - } - return BillingPlan.FREE; -} +function getPlanFromUrl(url: URL): string { + return url.searchParams.get('plan') ?? BillingPlan.FREE; +}src/routes/+page.ts (1)
37-49: Inconsistent path construction betweenresolve()andbase.Line 39 uses
resolve('/')while lines 43-48 use${base}/.... This inconsistency could lead to different behavior depending on how the app is deployed.Consider standardizing on one approach throughout the function for maintainability:
if (userVisitedEducationPage()) { await handleGithubEducationMembership(account.name, account.email); redirect(303, resolve('/')); } else if (organizations.total && !isApplyingCredit) { const teamId = account.prefs.organization ?? organizations.teams[0].$id; if (!teamId) { - redirect(303, `${base}/account/organizations${url.search}`); + redirect(303, `${resolve('/(console)/account/organizations')}${url.search}`); } else { - redirect(303, `${base}/organization-${teamId}${url.search}`); + redirect(303, `${resolve(`/(console)/organization-${teamId}`)}${url.search}`); } } else if (!isApplyingCredit) { - redirect(303, `${base}/onboarding/create-project${url.search}`); + redirect(303, `${resolve('/(console)/onboarding/create-project')}${url.search}`); }src/routes/(console)/account/organizations/+page.svelte (2)
40-47: Stale comment references old function name.The comment on line 41 says "use tierToPlan" but the code now uses
billingIdToPlan.🔎 Proposed fix
- // For known plans, use tierToPlan + // For known plans, use billingIdToPlan const tierData = billingIdToPlan(billingPlan);
76-84: Union typeModels.Preferences | Models.Organizationseems overly broad.The function accepts
Models.Preferencesbut then passes the value toisCloudOrgandisOrganizationOnTrialwhich expectModels.Organization. This works due to the type guard, but the signature is misleading since the function only returns a meaningful result forModels.Organization.Consider narrowing the parameter type or documenting why
Preferencesis accepted:function isPayingOrganization( - team: Models.Preferences | Models.Organization + team: Models.Organization ): Models.Organization | null {src/routes/(console)/organization-[organization]/billing/paymentMethods.svelte (1)
88-104: Missing error handling in reactive payment method fetches.The reactive blocks that fetch payment methods don't handle errors. If the API calls fail, the promises are silently rejected which could leave the UI in an inconsistent state.
🔎 Proposed fix
$: if (organization?.backupPaymentMethodId) { sdk.forConsole.organizations .getPaymentMethod({ organizationId: organization.$id, paymentMethodId: organization.backupPaymentMethodId }) - .then((res) => (backupPaymentMethod = res)); + .then((res) => (backupPaymentMethod = res)) + .catch(() => (backupPaymentMethod = null)); } $: if (organization?.paymentMethodId) { sdk.forConsole.organizations .getPaymentMethod({ organizationId: organization.$id, paymentMethodId: organization.paymentMethodId }) - .then((res) => (defaultPaymentMethod = res)); + .then((res) => (defaultPaymentMethod = res)) + .catch(() => (defaultPaymentMethod = null)); }src/routes/(console)/organization-[organization]/change-plan/+page.ts (1)
31-36: Address the TODO or remove it.The TODO comment questions the current implementation logic. If there's a known simplification, consider implementing it; otherwise, add context or remove the comment.
src/routes/(console)/organization-[organization]/billing/retryPaymentModal.svelte (1)
127-134: Inconsistentinvalidateawait pattern.Line 97 awaits
invalidate(Dependencies.PAYMENT_METHODS), but lines 133-134 do not await the invalidation calls. Consider awaiting these for consistency, or if intentional fire-and-forget, add a brief comment explaining why.🔎 Suggested fix for consistent await pattern
- invalidate(Dependencies.ORGANIZATION); - invalidate(Dependencies.INVOICES); + await invalidate(Dependencies.ORGANIZATION); + await invalidate(Dependencies.INVOICES);src/lib/stores/stripe.ts (1)
88-94: Hardcoded base URL should use environment configuration.The URL
https://cloud.appwrite.io/consoleis hardcoded here and again at line 226. Consider using an environment variable or the existingbaseimport from$app/pathscombined withwindow.location.originfor consistency and flexibility across environments.🔎 Suggested approach
- const baseUrl = 'https://cloud.appwrite.io/console'; + const baseUrl = `${window.location.origin}${base}`; const accountUrl = `${baseUrl}/account/payments?clientSecret=${clientSecret}`; const orgUrl = `${baseUrl}/organization-${organizationId}/billing?clientSecret=${clientSecret}`;Apply the same pattern at line 226.
src/lib/components/billing/estimatedTotalBox.svelte (1)
27-31: Consider consistent couponId null handling.The two estimation functions handle empty couponId differently:
getEstimate:couponId === '' ? null : couponIdgetUpdatePlanEstimate:couponId && couponId.length > 0 ? couponId : nullBoth approaches work, but the first could pass
undefinedas-is while the second explicitly converts tonull. Consider unifying the logic for maintainability.🔎 Suggested unified approach
async function getEstimate( billingPlan: string, collaborators: string[], couponId: string | undefined ) { try { estimation = await sdk.forConsole.organizations.estimationCreateOrganization({ billingPlan, invites: collaborators ?? [], - couponId: couponId === '' ? null : couponId + couponId: couponId?.length ? couponId : null });Also applies to: 57-62
src/routes/(console)/onboarding/create-project/+page.ts (1)
64-64: Consider using union type for nullable assignment.The variable is typed as
Models.ProjectListbut initialized tonull. While TypeScript non-strict mode tolerates this, a more accurate type would beModels.ProjectList | null.🔎 Proposed fix
-let projects: Models.ProjectList = null; +let projects: Models.ProjectList | null = null;
What does this PR do?
(Provide a description of what this PR does.)
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work.)
Related PRs and Issues
(If this PR is related to any other PR or resolves any issue or related to any issue link all related PR and issues here.)
Have you read the Contributing Guidelines on issues?
(Write your answer here.)
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.