Open
Conversation
- Updated the version of @appwrite.io/console in bun.lock and package.json. - Enhanced BAA settings page to handle missing or invalid addonId gracefully. - Added functionality to cancel and retry payment for pending BAA addon. - Improved BAAEnableModal with prorated pricing calculation and updated agreement terms.
Console (appwrite/console)Project ID: Tip Roll back Sites deployments instantly by switching between versions |
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:
WalkthroughAdds addon management for HIPAA BAA: introduces Dependencies.ADDONS and two analytics enum members (Submit.BAAAddonEnable, Submit.BAAAddonDisable). Updates a package dependency reference. Billing and settings load functions now depend on and fetch addons. Replaces the previous BAAModal with a new BAA UI component plus BAAEnableModal and BAADisableModal, including enable/disable flows, prorated pricing, validation via URL, cache invalidation, notifications, and analytics tracking. Adds addon-aware rendering in planSummary and minor import alias adjustments. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![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: 1
🧹 Nitpick comments (6)
src/routes/(console)/organization-[organization]/settings/BAADisableModal.svelte (1)
14-14: Type annotation should includenull.The
errorvariable is initialized tonullbut typed asstring. This could cause TypeScript issues in strict mode.♻️ Suggested fix
- let error: string = null; + let error: string | null = null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/`(console)/organization-[organization]/settings/BAADisableModal.svelte at line 14, The variable error is declared as let error: string = null which conflicts with strict TypeScript nullability rules; update the declaration for the BAADisableModal component so the type allows null (e.g., change error's type to string | null or use string | undefined) and ensure any downstream uses handle the nullable type (adjust checks or non-null assertions where appropriate).src/routes/(console)/organization-[organization]/settings/BAAEnableModal.svelte (3)
86-96: Silent handling of 409 may confuse users.When a 409 (addon already exists) occurs, the modal silently closes without informing the user. Consider adding a notification to clarify that the addon was already enabled.
♻️ Suggested improvement
// 409 means addon already exists (pending or active from prior attempt) if (e?.code === 409) { await Promise.all([ invalidate(Dependencies.ADDONS), invalidate(Dependencies.ORGANIZATION) ]); + addNotification({ + message: 'BAA addon is already enabled or pending', + type: 'info' + }); show = false; } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/`(console)/organization-[organization]/settings/BAAEnableModal.svelte around lines 86 - 96, When handling the 409 branch in the BAA enable flow (the block checking e?.code === 409), don't silently close the modal — add a user-facing notification or set a visible message before hiding it so the user knows the addon was already enabled; update the same branch that currently invalidates Dependencies.ADDONS and Dependencies.ORGANIZATION and sets show = false to also call the app's notification method (or set error/toast state) with a clear message like "BAA addon already enabled" and still call trackError/metrics as appropriate (refer to e?.code, show, Dependencies.ADDONS, Dependencies.ORGANIZATION, trackError, and Submit.BAAAddonEnable to locate the exact spot).
20-20: Type annotation should includenull.Same issue as in
BAADisableModal.svelte- theerrorvariable is initialized tonullbut typed asstring.♻️ Suggested fix
- let error: string | null = null; + let error: string | null = null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/`(console)/organization-[organization]/settings/BAAEnableModal.svelte at line 20, The variable `error` in BAAEnableModal.svelte is declared as `let error: string = null;` but allows null; update its type to include null (e.g., `string | null`) so the annotation matches the initializer and mirror the fix applied in BAADisableModal.svelte; change the type annotation on the `error` variable declaration (`let error`) accordingly throughout the component where referenced.
60-72: Type assertion could be avoided with better type narrowing.The
as unknown as Models.PaymentAuthenticationdouble cast is a TypeScript workaround. The API response type should ideally be a discriminated union that TypeScript can narrow properly.♻️ Consider cleaner type handling
if ('clientSecret' in result) { - const paymentAuth = result as unknown as Models.PaymentAuthentication; + // Result is PaymentAuthentication when clientSecret is present + const paymentAuth = result as Models.PaymentAuthentication; const settingsUrl = resolve(If TypeScript still complains, the SDK types may need updating to properly represent the union return type.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/`(console)/organization-[organization]/settings/BAAEnableModal.svelte around lines 60 - 72, The double cast to Models.PaymentAuthentication should be removed and replaced with proper type narrowing: add a type guard function (e.g. isPaymentAuthentication(obj): obj is Models.PaymentAuthentication) that checks for the discriminant properties used here (e.g. 'clientSecret' and 'addonId'), then use that guard in the existing if ('clientSecret' in result) branch so you can assign paymentAuth = result without the "as unknown as" cast and call confirmPayment({ clientSecret: paymentAuth.clientSecret, paymentMethodId: $organization.paymentMethodId, orgId: $organization.$id, route: `${settingsUrl}?type=validate-addon&addonId=${paymentAuth.addonId}` }); if the SDK types remain incorrect, update the SDK return type to a proper discriminated union so TypeScript can narrow automatically.src/routes/(console)/organization-[organization]/settings/+page.ts (1)
16-27: Minor inconsistency in nullish value handling.The
invoicesbranch returnsundefinedfor non-cloud (line 20), whileaddonsreturnsnull(line 27). Consider using a consistent approach for both.♻️ Suggested consistency fix
isCloud ? sdk.forConsole.organizations.listInvoices({ organizationId: params.organization }) - : undefined, + : null, isCloud ? sdk.forConsole.organizations .listAddons({ organizationId: params.organization }) .catch(() => null) : null🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/`(console)/organization-[organization]/settings/+page.ts around lines 16 - 27, The code inconsistency returns undefined for the invoices branch but null for the addons branch when isCloud is false; update one of them so both non-cloud branches return the same nullish value (pick either undefined or null) — modify the expressions around isCloud ? sdk.forConsole.organizations.listInvoices({ organizationId: params.organization }) : undefined and isCloud ? sdk.forConsole.organizations.listAddons({ organizationId: params.organization }).catch(() => null) : null so both use the same fallback (e.g., change the invoices fallback to null or the addons fallback to undefined and adjust the catch to return the chosen value), keeping references to listInvoices, listAddons, isCloud, and params.organization intact.src/routes/(console)/organization-[organization]/settings/BAA.svelte (1)
15-16: Use$routesaliases for these modal imports.Switching these to route aliases keeps import style consistent with repository conventions.
Proposed refactor
-import BAAEnableModal from './BAAEnableModal.svelte'; -import BAADisableModal from './BAADisableModal.svelte'; +import BAAEnableModal from '$routes/(console)/organization-[organization]/settings/BAAEnableModal.svelte'; +import BAADisableModal from '$routes/(console)/organization-[organization]/settings/BAADisableModal.svelte';As per coding guidelines
**/*.{js,ts,svelte}: Use $lib, $routes, and $themes path aliases for imports.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/`(console)/organization-[organization]/settings/BAA.svelte around lines 15 - 16, Replace the relative imports for the two modal components with the repository route alias: change the current relative imports of BAAEnableModal and BAADisableModal to use the $routes alias so they follow project conventions (import BAAEnableModal from '$routes/...'; import BAADisableModal from '$routes/...'), ensuring the module specifiers point to the same component files and update any import paths accordingly in the BAA.svelte file where BAAEnableModal and BAADisableModal are referenced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/routes/`(console)/organization-[organization]/settings/+page.svelte:
- Around line 35-45: The current try/catch around
sdk.forConsole.organizations.listAddons swallows errors and sets addonId = null,
which later causes the UI to show “BAA addon has been enabled” incorrectly;
instead, catch should record the failure (e.g., set an addonLookupError flag or
store the caught error) rather than conflating “no addon” and “lookup failed”,
and update the UI logic that reads addonId/pending to check this failure flag
before rendering the success/fallback branch; specifically, modify the block
using listAddons, pending, and addonId so the catch saves the error (or boolean)
and the downstream rendering avoids reporting success when addonLookupError is
set.
---
Nitpick comments:
In `@src/routes/`(console)/organization-[organization]/settings/+page.ts:
- Around line 16-27: The code inconsistency returns undefined for the invoices
branch but null for the addons branch when isCloud is false; update one of them
so both non-cloud branches return the same nullish value (pick either undefined
or null) — modify the expressions around isCloud ?
sdk.forConsole.organizations.listInvoices({ organizationId: params.organization
}) : undefined and isCloud ? sdk.forConsole.organizations.listAddons({
organizationId: params.organization }).catch(() => null) : null so both use the
same fallback (e.g., change the invoices fallback to null or the addons fallback
to undefined and adjust the catch to return the chosen value), keeping
references to listInvoices, listAddons, isCloud, and params.organization intact.
In `@src/routes/`(console)/organization-[organization]/settings/BAA.svelte:
- Around line 15-16: Replace the relative imports for the two modal components
with the repository route alias: change the current relative imports of
BAAEnableModal and BAADisableModal to use the $routes alias so they follow
project conventions (import BAAEnableModal from '$routes/...'; import
BAADisableModal from '$routes/...'), ensuring the module specifiers point to the
same component files and update any import paths accordingly in the BAA.svelte
file where BAAEnableModal and BAADisableModal are referenced.
In
`@src/routes/`(console)/organization-[organization]/settings/BAADisableModal.svelte:
- Line 14: The variable error is declared as let error: string = null which
conflicts with strict TypeScript nullability rules; update the declaration for
the BAADisableModal component so the type allows null (e.g., change error's type
to string | null or use string | undefined) and ensure any downstream uses
handle the nullable type (adjust checks or non-null assertions where
appropriate).
In
`@src/routes/`(console)/organization-[organization]/settings/BAAEnableModal.svelte:
- Around line 86-96: When handling the 409 branch in the BAA enable flow (the
block checking e?.code === 409), don't silently close the modal — add a
user-facing notification or set a visible message before hiding it so the user
knows the addon was already enabled; update the same branch that currently
invalidates Dependencies.ADDONS and Dependencies.ORGANIZATION and sets show =
false to also call the app's notification method (or set error/toast state) with
a clear message like "BAA addon already enabled" and still call
trackError/metrics as appropriate (refer to e?.code, show, Dependencies.ADDONS,
Dependencies.ORGANIZATION, trackError, and Submit.BAAAddonEnable to locate the
exact spot).
- Line 20: The variable `error` in BAAEnableModal.svelte is declared as `let
error: string = null;` but allows null; update its type to include null (e.g.,
`string | null`) so the annotation matches the initializer and mirror the fix
applied in BAADisableModal.svelte; change the type annotation on the `error`
variable declaration (`let error`) accordingly throughout the component where
referenced.
- Around line 60-72: The double cast to Models.PaymentAuthentication should be
removed and replaced with proper type narrowing: add a type guard function (e.g.
isPaymentAuthentication(obj): obj is Models.PaymentAuthentication) that checks
for the discriminant properties used here (e.g. 'clientSecret' and 'addonId'),
then use that guard in the existing if ('clientSecret' in result) branch so you
can assign paymentAuth = result without the "as unknown as" cast and call
confirmPayment({ clientSecret: paymentAuth.clientSecret, paymentMethodId:
$organization.paymentMethodId, orgId: $organization.$id, route:
`${settingsUrl}?type=validate-addon&addonId=${paymentAuth.addonId}` }); if the
SDK types remain incorrect, update the SDK return type to a proper discriminated
union so TypeScript can narrow automatically.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
package.jsonsrc/lib/actions/analytics.tssrc/lib/constants.tssrc/routes/(console)/organization-[organization]/billing/+page.tssrc/routes/(console)/organization-[organization]/billing/planSummary.sveltesrc/routes/(console)/organization-[organization]/settings/+page.sveltesrc/routes/(console)/organization-[organization]/settings/+page.tssrc/routes/(console)/organization-[organization]/settings/BAA.sveltesrc/routes/(console)/organization-[organization]/settings/BAADisableModal.sveltesrc/routes/(console)/organization-[organization]/settings/BAAEnableModal.sveltesrc/routes/(console)/organization-[organization]/settings/BAAModal.svelte
💤 Files with no reviewable changes (1)
- src/routes/(console)/organization-[organization]/settings/BAAModal.svelte
Comment on lines
+35
to
+45
| try { | ||
| const addons = await sdk.forConsole.organizations.listAddons({ | ||
| organizationId: $organization.$id | ||
| }); | ||
| const pending = addons.addons.find( | ||
| (a) => a.key === 'baa' && a.status === 'pending' | ||
| ); | ||
| addonId = pending?.$id ?? null; | ||
| } catch { | ||
| addonId = null; | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid reporting success when addon lookup fails.
If listAddons fails (network/5xx/auth), the code sets addonId = null and then shows “BAA addon has been enabled” in the fallback branch. That can hide real failures and mislead users.
Proposed fix
onMount(async () => {
name = $organization.name;
if (page.url.searchParams.get('type') === 'validate-addon') {
let addonId = page.url.searchParams.get('addonId');
+ let lookupFailed = false;
// Fall back to listing addons if addonId is missing or invalid
if (!addonId || addonId === 'undefined') {
try {
const addons = await sdk.forConsole.organizations.listAddons({
organizationId: $organization.$id
});
const pending = addons.addons.find(
(a) => a.key === 'baa' && a.status === 'pending'
);
addonId = pending?.$id ?? null;
- } catch {
- addonId = null;
+ } catch (e) {
+ lookupFailed = true;
+ addNotification({
+ message: e?.message ?? 'Unable to verify BAA addon status. Please retry.',
+ type: 'error'
+ });
}
}
+
+ if (lookupFailed) {
+ const settingsUrl = resolve('/(console)/organization-[organization]/settings', {
+ organization: $organization.$id
+ });
+ await goto(settingsUrl, { replaceState: true });
+ return;
+ }
if (addonId) {
// existing logic...Also applies to: 80-89
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routes/`(console)/organization-[organization]/settings/+page.svelte
around lines 35 - 45, The current try/catch around
sdk.forConsole.organizations.listAddons swallows errors and sets addonId = null,
which later causes the UI to show “BAA addon has been enabled” incorrectly;
instead, catch should record the failure (e.g., set an addonLookupError flag or
store the caught error) rather than conflating “no addon” and “lookup failed”,
and update the UI logic that reads addonId/pending to check this failure flag
before rendering the success/fallback branch; specifically, modify the block
using listAddons, pending, and addonId so the catch saves the error (or boolean)
and the downstream rendering avoids reporting success when addonLookupError is
set.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/routes/(console)/organization-[organization]/settings/BAAEnableModal.svelte (2)
60-73: Consider refining the type narrowing for payment authentication.The
as unknown asdouble assertion is a type-safety bypass. Since the API returns a union type, you could use a type guard or rely on the discriminant property check more cleanly.Suggested approach using type predicate
+ function isPaymentAuth(result: Models.Addon | Models.PaymentAuthentication): result is Models.PaymentAuthentication { + return 'clientSecret' in result; + } async function handleSubmit() { // ... - if ('clientSecret' in result) { - const paymentAuth = result as unknown as Models.PaymentAuthentication; + if (isPaymentAuth(result)) { const settingsUrl = resolve( '/(console)/organization-[organization]/settings', { organization: $organization.$id } ); await confirmPayment({ - clientSecret: paymentAuth.clientSecret, + clientSecret: result.clientSecret, paymentMethodId: $organization.paymentMethodId, orgId: $organization.$id, - route: `${settingsUrl}?type=validate-addon&addonId=${paymentAuth.addonId}` + route: `${settingsUrl}?type=validate-addon&addonId=${result.addonId}` }); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/`(console)/organization-[organization]/settings/BAAEnableModal.svelte around lines 60 - 73, The code uses an unsafe double cast ("as unknown as Models.PaymentAuthentication") to treat result as a PaymentAuthentication; replace this with a proper type guard that narrows the union safely (e.g. add a function isPaymentAuthentication(obj): obj is Models.PaymentAuthentication that checks for discriminant properties like 'clientSecret' and 'addonId'), then use if (isPaymentAuthentication(result)) { const paymentAuth = result; ... confirmPayment(...) } and remove the "as unknown as" assertion; update the block around result and the confirmPayment call to rely on the type guard.
85-96: Add type guard for error properties.Accessing
e.messagewithout a type check can fail if the caught value isn't an Error object. Consider narrowing the type.Proposed fix
} catch (e) { // 409 means addon already exists (pending or active from prior attempt) - if (e?.code === 409) { + const err = e as { code?: number; message?: string }; + if (err?.code === 409) { await Promise.all([ invalidate(Dependencies.ADDONS), invalidate(Dependencies.ORGANIZATION) ]); show = false; } else { - error = e.message; + error = err?.message ?? 'An unexpected error occurred'; trackError(e, Submit.BAAAddonEnable); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/`(console)/organization-[organization]/settings/BAAEnableModal.svelte around lines 85 - 96, The catch block accesses e?.code and e.message without narrowing the caught value; add a type guard and normalize the error before using its properties. Replace direct checks with a safe guard like: determine isConflict by checking e is an object and has a code property (e && typeof (e as any).code !== 'undefined' && (e as any).code === 409), and derive error text with const errMessage = e instanceof Error ? e.message : String(e); then use isConflict to run invalidate/close modal and otherwise set error = errMessage and call trackError(e, Submit.BAAAddonEnable). Update references in this block (the catch, show, error, invalidate calls, and trackError invocation) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/routes/`(console)/organization-[organization]/settings/BAAEnableModal.svelte:
- Line 20: The variable declaration let error: string = null; has a type
mismatch because null is assigned; change the type to allow null (e.g., update
the declaration of error to let error: string | null = null) and ensure any
assignments in handleSubmit and other places (setting error = null or assigning
strings) are compatible with string | null.
---
Nitpick comments:
In
`@src/routes/`(console)/organization-[organization]/settings/BAAEnableModal.svelte:
- Around line 60-73: The code uses an unsafe double cast ("as unknown as
Models.PaymentAuthentication") to treat result as a PaymentAuthentication;
replace this with a proper type guard that narrows the union safely (e.g. add a
function isPaymentAuthentication(obj): obj is Models.PaymentAuthentication that
checks for discriminant properties like 'clientSecret' and 'addonId'), then use
if (isPaymentAuthentication(result)) { const paymentAuth = result; ...
confirmPayment(...) } and remove the "as unknown as" assertion; update the block
around result and the confirmPayment call to rely on the type guard.
- Around line 85-96: The catch block accesses e?.code and e.message without
narrowing the caught value; add a type guard and normalize the error before
using its properties. Replace direct checks with a safe guard like: determine
isConflict by checking e is an object and has a code property (e && typeof (e as
any).code !== 'undefined' && (e as any).code === 409), and derive error text
with const errMessage = e instanceof Error ? e.message : String(e); then use
isConflict to run invalidate/close modal and otherwise set error = errMessage
and call trackError(e, Submit.BAAAddonEnable). Update references in this block
(the catch, show, error, invalidate calls, and trackError invocation)
accordingly.
src/routes/(console)/organization-[organization]/settings/BAAEnableModal.svelte
Outdated
Show resolved
Hide resolved
Contributor
Greptile SummaryThis PR replaces the old manual BAA request form with a full self-service addon management UI — enabling, disabling, re-enabling, and handling 3DS payment authentication for the HIPAA BAA addon. The implementation covers the complete lifecycle including prorated billing, status badges, and a return-URL validation flow for Stripe 3DS redirects. Key findings:
Confidence Score: 3/5
Last reviewed commit: 165cea1 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
src/routes/(console)/organization-[organization]/settings/+page.svelte
Outdated
Show resolved
Hide resolved
src/routes/(console)/organization-[organization]/billing/planSummary.svelte
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/routes/(console)/organization-[organization]/settings/+page.svelte (1)
34-45:⚠️ Potential issue | 🟠 MajorDon’t convert addon lookup failures into success UX.
At Line 43-45, lookup errors are swallowed by forcing
addonId = null, which then falls into the success path at Line 80-89 and shows “BAA addon has been enabled” even when lookup failed.Proposed fix
if (page.url.searchParams.get('type') === 'validate-addon') { let addonId = page.url.searchParams.get('addonId'); + let lookupFailed = false; // Fall back to listing addons if addonId is missing or invalid if (!addonId || addonId === 'undefined') { try { const addons = await sdk.forConsole.organizations.listAddons({ organizationId: $organization.$id }); const pending = addons.addons.find( (a) => a.key === 'baa' && a.status === 'pending' ); addonId = pending?.$id ?? null; - } catch { - addonId = null; + } catch (e) { + lookupFailed = true; + addNotification({ + message: e?.message ?? 'Unable to verify BAA addon status. Please retry.', + type: 'error' + }); } } + + if (lookupFailed) { + const settingsUrl = resolve('/(console)/organization-[organization]/settings', { + organization: $organization.$id + }); + await goto(settingsUrl, { replaceState: true }); + return; + }Also applies to: 80-89
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/`(console)/organization-[organization]/settings/+page.svelte around lines 34 - 45, The try/catch that resolves addonId (using sdk.forConsole.organizations.listAddons and the pending lookup into pending?.$id) currently swallows lookup errors by setting addonId = null which later triggers the success UI that shows "BAA addon has been enabled"; instead, surface the error so the success path won’t run: in the catch, record an explicit failure state (e.g., set an error flag or rethrow) and ensure the downstream rendering logic that displays the "BAA addon has been enabled" message checks that no lookup error occurred before showing success; update the code around the addonId resolution and the success-rendering branch to consult this error flag (or propagate the exception) rather than treating null as "not enabled."src/routes/(console)/organization-[organization]/settings/BAAEnableModal.svelte (1)
20-20:⚠️ Potential issue | 🟡 MinorFix nullable
errortyping (still unresolved).At Line 20,
erroris declared asstringbut is assignednull. Usestring | null.Proposed fix
- let error: string = null; + let error: string | null = null;#!/bin/bash # Verify nullable typing mismatches in Svelte files rg -nP 'let\s+error:\s*string\s*=\s*null;' --type svelte🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/`(console)/organization-[organization]/settings/BAAEnableModal.svelte at line 20, The variable error in BAAEnableModal.svelte is typed as string but initialized to null; change its type to allow null by updating the declaration of error (the let error variable) to use the nullable union type string | null so the initialization is valid and TypeScript type-checks pass.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/routes/`(console)/organization-[organization]/settings/BAADisableModal.svelte:
- Line 14: The variable error in BAADisableModal.svelte is declared as let
error: string = null but is initialized and later reset to null; change its type
to allow null by declaring it as string | null so TypeScript is type-safe —
update the declaration of error (the let error variable) to use the union type
string | null and ensure any assignments that set it to null remain valid.
---
Duplicate comments:
In `@src/routes/`(console)/organization-[organization]/settings/+page.svelte:
- Around line 34-45: The try/catch that resolves addonId (using
sdk.forConsole.organizations.listAddons and the pending lookup into
pending?.$id) currently swallows lookup errors by setting addonId = null which
later triggers the success UI that shows "BAA addon has been enabled"; instead,
surface the error so the success path won’t run: in the catch, record an
explicit failure state (e.g., set an error flag or rethrow) and ensure the
downstream rendering logic that displays the "BAA addon has been enabled"
message checks that no lookup error occurred before showing success; update the
code around the addonId resolution and the success-rendering branch to consult
this error flag (or propagate the exception) rather than treating null as "not
enabled."
In
`@src/routes/`(console)/organization-[organization]/settings/BAAEnableModal.svelte:
- Line 20: The variable error in BAAEnableModal.svelte is typed as string but
initialized to null; change its type to allow null by updating the declaration
of error (the let error variable) to use the nullable union type string | null
so the initialization is valid and TypeScript type-checks pass.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
package.jsonsrc/routes/(console)/organization-[organization]/billing/planSummary.sveltesrc/routes/(console)/organization-[organization]/settings/+page.sveltesrc/routes/(console)/organization-[organization]/settings/BAA.sveltesrc/routes/(console)/organization-[organization]/settings/BAADisableModal.sveltesrc/routes/(console)/organization-[organization]/settings/BAAEnableModal.svelte
🚧 Files skipped from review as they are similar to previous changes (2)
- src/routes/(console)/organization-[organization]/billing/planSummary.svelte
- package.json
| export let show = false; | ||
| export let addonId: string; | ||
|
|
||
| let error: string = null; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and read its content
if [ -f "src/routes/(console)/organization-[organization]/settings/BAADisableModal.svelte" ]; then
echo "=== File found, reading content ==="
cat -n "src/routes/(console)/organization-[organization]/settings/BAADisableModal.svelte"
else
echo "File not found at exact path, trying to find it..."
find . -name "BAADisableModal.svelte" -type f 2>/dev/null
fiRepository: appwrite/console
Length of output: 2367
Fix nullable error typing.
At line 14, error is typed as string but initialized to null and can be reset to null (line 19) in the try block. Change the type to string | null to be type-safe.
Proposed fix
- let error: string = null;
+ let error: string | null = null;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let error: string = null; | |
| let error: string | null = null; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/routes/`(console)/organization-[organization]/settings/BAADisableModal.svelte
at line 14, The variable error in BAADisableModal.svelte is declared as let
error: string = null but is initialized and later reset to null; change its type
to allow null by declaring it as string | null so TypeScript is type-safe —
update the declaration of error (the let error variable) to use the union type
string | null and ensure any assignments that set it to null remain valid.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/routes/(console)/organization-[organization]/settings/+page.svelte (1)
34-45:⚠️ Potential issue | 🟠 MajorDon’t silently downgrade addon lookup failures to “no addon found.”
When
listAddonsfails (Line 43), the code setsaddonId = nulland continues to redirect (Line 82), so validation is skipped with no user-visible failure signal. Keep lookup failure distinct from “no pending addon” and short-circuit after notifying.Suggested fix
if (page.url.searchParams.get('type') === 'validate-addon') { let addonId = page.url.searchParams.get('addonId'); + let addonLookupFailed = false; // Fall back to listing addons if addonId is missing or invalid if (!addonId || addonId === 'undefined') { try { const addons = await sdk.forConsole.organizations.listAddons({ organizationId: $organization.$id }); const pending = addons.addons.find( (a) => a.key === 'baa' && a.status === 'pending' ); addonId = pending?.$id ?? null; - } catch { - addonId = null; + } catch (e) { + addonLookupFailed = true; + addNotification({ + message: e?.message ?? 'Unable to verify BAA addon status. Please retry.', + type: 'error' + }); + trackError(e, Submit.BAAAddonEnable); } } + + if (addonLookupFailed) { + const settingsUrl = resolve('/(console)/organization-[organization]/settings', { + organization: $organization.$id + }); + await goto(settingsUrl, { replaceState: true }); + return; + } if (addonId) {Also applies to: 82-85
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/`(console)/organization-[organization]/settings/+page.svelte around lines 34 - 45, The try/catch around sdk.forConsole.organizations.listAddons silently converts lookup failures into "no addon" by setting addonId = null; instead, preserve lookup errors by either rethrowing the error or setting a lookupFailed flag and returning early so the page doesn't continue to the validation/redirect path. Update the catch block in the load logic that calls sdk.forConsole.organizations.listAddons (the block that assigns addonId and inspects pending) to log/report the caught error and short-circuit (e.g., throw the error or return an error result) rather than assigning addonId = null, so downstream logic that uses addonId or triggers the redirect can detect and show an error to the user.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/routes/`(console)/organization-[organization]/settings/BAAEnableModal.svelte:
- Around line 34-47: Guard against invalid invoice date strings by validating
parsed dates before doing proration math: after creating cycleStart and cycleEnd
from currentInvoiceDate and nextInvoiceDate, check isNaN(cycleStart.getTime())
or isNaN(cycleEnd.getTime()) and if invalid, fall back to safe defaults (e.g.,
set cycleStart = today or cycleEnd = new Date(today.getTime() +
30*24*60*60*1000)) or return 0 price; then compute totalDays and remainingDays
as before using these validated dates and ensure totalDays is at least 1 to
avoid NaN/divide-by-zero. Reference the variables cycleStart, cycleEnd,
totalDays, remainingDays, currentInvoiceDate, nextInvoiceDate, and
BAA_MONTHLY_PRICE when making the change.
---
Duplicate comments:
In `@src/routes/`(console)/organization-[organization]/settings/+page.svelte:
- Around line 34-45: The try/catch around
sdk.forConsole.organizations.listAddons silently converts lookup failures into
"no addon" by setting addonId = null; instead, preserve lookup errors by either
rethrowing the error or setting a lookupFailed flag and returning early so the
page doesn't continue to the validation/redirect path. Update the catch block in
the load logic that calls sdk.forConsole.organizations.listAddons (the block
that assigns addonId and inspects pending) to log/report the caught error and
short-circuit (e.g., throw the error or return an error result) rather than
assigning addonId = null, so downstream logic that uses addonId or triggers the
redirect can detect and show an error to the user.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/routes/(console)/organization-[organization]/billing/planSummary.sveltesrc/routes/(console)/organization-[organization]/settings/+page.sveltesrc/routes/(console)/organization-[organization]/settings/BAAEnableModal.svelte
🚧 Files skipped from review as they are similar to previous changes (1)
- src/routes/(console)/organization-[organization]/billing/planSummary.svelte
Comment on lines
+34
to
+47
| const today = new Date(); | ||
| const cycleStart = new Date(currentInvoiceDate); | ||
| const cycleEnd = new Date(nextInvoiceDate); | ||
|
|
||
| const totalDays = Math.max( | ||
| 1, | ||
| Math.ceil((cycleEnd.getTime() - cycleStart.getTime()) / (1000 * 60 * 60 * 24)) | ||
| ); | ||
| const remainingDays = Math.max( | ||
| 0, | ||
| Math.ceil((cycleEnd.getTime() - today.getTime()) / (1000 * 60 * 60 * 24)) | ||
| ); | ||
|
|
||
| return Math.round(((BAA_MONTHLY_PRICE * remainingDays) / totalDays) * 100) / 100; |
Contributor
There was a problem hiding this comment.
Guard against invalid invoice date strings in proration math.
If either parsed date is invalid, arithmetic returns NaN, which can surface as an invalid currency value in the modal. Add a parse-validity fallback.
Suggested fix
const today = new Date();
const cycleStart = new Date(currentInvoiceDate);
const cycleEnd = new Date(nextInvoiceDate);
+
+ if (Number.isNaN(cycleStart.getTime()) || Number.isNaN(cycleEnd.getTime())) {
+ return BAA_MONTHLY_PRICE;
+ }
const totalDays = Math.max(
1,
Math.ceil((cycleEnd.getTime() - cycleStart.getTime()) / (1000 * 60 * 60 * 24))
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const today = new Date(); | |
| const cycleStart = new Date(currentInvoiceDate); | |
| const cycleEnd = new Date(nextInvoiceDate); | |
| const totalDays = Math.max( | |
| 1, | |
| Math.ceil((cycleEnd.getTime() - cycleStart.getTime()) / (1000 * 60 * 60 * 24)) | |
| ); | |
| const remainingDays = Math.max( | |
| 0, | |
| Math.ceil((cycleEnd.getTime() - today.getTime()) / (1000 * 60 * 60 * 24)) | |
| ); | |
| return Math.round(((BAA_MONTHLY_PRICE * remainingDays) / totalDays) * 100) / 100; | |
| const today = new Date(); | |
| const cycleStart = new Date(currentInvoiceDate); | |
| const cycleEnd = new Date(nextInvoiceDate); | |
| if (Number.isNaN(cycleStart.getTime()) || Number.isNaN(cycleEnd.getTime())) { | |
| return BAA_MONTHLY_PRICE; | |
| } | |
| const totalDays = Math.max( | |
| 1, | |
| Math.ceil((cycleEnd.getTime() - cycleStart.getTime()) / (1000 * 60 * 60 * 24)) | |
| ); | |
| const remainingDays = Math.max( | |
| 0, | |
| Math.ceil((cycleEnd.getTime() - today.getTime()) / (1000 * 60 * 60 * 24)) | |
| ); | |
| return Math.round(((BAA_MONTHLY_PRICE * remainingDays) / totalDays) * 100) / 100; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/routes/`(console)/organization-[organization]/settings/BAAEnableModal.svelte
around lines 34 - 47, Guard against invalid invoice date strings by validating
parsed dates before doing proration math: after creating cycleStart and cycleEnd
from currentInvoiceDate and nextInvoiceDate, check isNaN(cycleStart.getTime())
or isNaN(cycleEnd.getTime()) and if invalid, fall back to safe defaults (e.g.,
set cycleStart = today or cycleEnd = new Date(today.getTime() +
30*24*60*60*1000)) or return 0 price; then compute totalDays and remainingDays
as before using these validated dates and ensure totalDays is at least 1 to
avoid NaN/divide-by-zero. Reference the variables cycleStart, cycleEnd,
totalDays, remainingDays, currentInvoiceDate, nextInvoiceDate, and
BAA_MONTHLY_PRICE when making the change.
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 `@src/lib/stores/migration.ts`:
- Line 2: The current import aliases AppwriteMigrationResource as Resources
which narrows providerResources to Appwrite-only enums and breaks calls to
getSupabaseReport, getFirebaseReport, and getNHostReport from
resource-form.svelte; fix by importing the provider-specific enum types (e.g.,
AppwriteMigrationResource, SupabaseMigrationResource, FirebaseMigrationResource,
NHostMigrationResource) from `@appwrite.io/console` and change the
providerResources type to a mapped/union type keyed by provider (e.g., a
ProviderResourcesMap or a discriminated union) so each provider maps to its own
enum; update any references to Resources in migration.ts and the
resource-form.svelte parameters to use the new mapped type so
getSupabaseReport/getFirebaseReport/getNHostReport receive the correct enum
values.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
package.jsonsrc/lib/stores/migration.tssrc/routes/(console)/(migration-wizard)/resource-form.svelte
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
Comment on lines
+120
to
+127
| </div> | ||
| <hr class="divider" /> | ||
| <div class="price-row u-bold"> | ||
| <span class="text">Total</span> | ||
| <span class="text">{formatCurrency(BAA_MONTHLY_PRICE)} / month</span> | ||
| </div> | ||
| <p class="text u-color-text-offline u-margin-block-start-8 u-text-end"> | ||
| * Plus applicable tax and fees |
Contributor
There was a problem hiding this comment.
Price breakdown "Total" shows monthly rate, not immediate prorated charge
The introductory paragraph above correctly tells the user they will be charged {proratedAmount} immediately, but the "Total" row in the breakdown repeats the full BAA_MONTHLY_PRICE ($350) instead of the prorated amount. A user who checks the breakdown to confirm the charge they are about to authorize will see a different (higher) figure there. For a legally-binding agreement like a BAA, showing the actual immediate charge prominently in the breakdown is important.
Suggested change
| </div> | |
| <hr class="divider" /> | |
| <div class="price-row u-bold"> | |
| <span class="text">Total</span> | |
| <span class="text">{formatCurrency(BAA_MONTHLY_PRICE)} / month</span> | |
| </div> | |
| <p class="text u-color-text-offline u-margin-block-start-8 u-text-end"> | |
| * Plus applicable tax and fees | |
| <div class="price-row u-bold"> | |
| <span class="text">Due today (prorated)</span> | |
| <span class="text">{formatCurrency(proratedAmount)}</span> | |
| </div> |
Comment on lines
+84
to
+95
| } catch (e) { | ||
| // 409 means addon already exists (pending or active from prior attempt) | ||
| if (e?.code === 409) { | ||
| await Promise.all([ | ||
| invalidate(Dependencies.ADDONS), | ||
| invalidate(Dependencies.ORGANIZATION) | ||
| ]); | ||
| show = false; | ||
| } else { | ||
| error = e.message; | ||
| trackError(e, Submit.BAAAddonEnable); | ||
| } |
Contributor
There was a problem hiding this comment.
Silent modal close on 409 provides no user feedback
When createBaaAddon returns a 409 (addon already exists), the modal closes silently — no success notification is shown. The user has no indication that the BAA is already active/pending. At minimum a notification should be shown so the user understands what happened.
Suggested change
| } catch (e) { | |
| // 409 means addon already exists (pending or active from prior attempt) | |
| if (e?.code === 409) { | |
| await Promise.all([ | |
| invalidate(Dependencies.ADDONS), | |
| invalidate(Dependencies.ORGANIZATION) | |
| ]); | |
| show = false; | |
| } else { | |
| error = e.message; | |
| trackError(e, Submit.BAAAddonEnable); | |
| } | |
| } catch (e) { | |
| // 409 means addon already exists (pending or active from prior attempt) | |
| if (e?.code === 409) { | |
| await Promise.all([ | |
| invalidate(Dependencies.ADDONS), | |
| invalidate(Dependencies.ORGANIZATION) | |
| ]); | |
| addNotification({ | |
| message: 'BAA addon is already active for your organization', | |
| type: 'success' | |
| }); | |
| show = false; | |
| } else { | |
| error = e.message; | |
| trackError(e, Submit.BAAAddonEnable); | |
| } |
Comment on lines
+64
to
+89
| async function handleReEnable() { | ||
| reEnabling = true; | ||
| try { | ||
| await sdk.forConsole.organizations.createBaaAddon({ | ||
| organizationId: $organization.$id, | ||
| key: 'baa' | ||
| }); | ||
| await Promise.all([ | ||
| invalidate(Dependencies.ADDONS), | ||
| invalidate(Dependencies.ORGANIZATION) | ||
| ]); | ||
| addNotification({ | ||
| message: 'BAA addon has been re-enabled', | ||
| type: 'success' | ||
| }); | ||
| trackEvent(Submit.BAAAddonEnable); | ||
| } catch (e) { | ||
| addNotification({ | ||
| message: e.message, | ||
| type: 'error' | ||
| }); | ||
| trackError(e, Submit.BAAAddonEnable); | ||
| } finally { | ||
| reEnabling = false; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
handleReEnable ignores PaymentAuthentication response
createBaaAddon can return either a Models.Addon or a Models.PaymentAuthentication (when 3DS payment confirmation is required). BAAEnableModal.svelte's handleSubmit correctly guards against this with if ('clientSecret' in result) before proceeding, but handleReEnable discards the return value entirely and immediately shows a "BAA addon has been re-enabled" success notification.
If the API returns a PaymentAuthentication object, the addon will be left in a pending state while the user believes it has been fully re-enabled — the Stripe payment confirmation step is never initiated and the charge never completes. The same clientSecret-check and confirmPayment call used in BAAEnableModal.svelte should be added here.
Comment on lines
+48
to
+85
| if (addonId) { | ||
| try { | ||
| await sdk.forConsole.organizations.validateAddonPayment({ | ||
| organizationId: $organization.$id, | ||
| addonId | ||
| }); | ||
| await Promise.all([ | ||
| invalidate(Dependencies.ADDONS), | ||
| invalidate(Dependencies.ORGANIZATION) | ||
| ]); | ||
| addNotification({ | ||
| message: 'BAA addon has been enabled', | ||
| type: 'success' | ||
| }); | ||
| } catch (e) { | ||
| // If addon not found, payment webhook may have already activated it | ||
| if (e?.type === 'addon_not_found' || e?.code === 404) { | ||
| await Promise.all([ | ||
| invalidate(Dependencies.ADDONS), | ||
| invalidate(Dependencies.ORGANIZATION) | ||
| ]); | ||
| addNotification({ | ||
| message: 'BAA addon has been enabled', | ||
| type: 'success' | ||
| }); | ||
| } else { | ||
| addNotification({ | ||
| message: e.message, | ||
| type: 'error' | ||
| }); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| const settingsUrl = resolve('/(console)/organization-[organization]/settings', { | ||
| organization: $organization.$id | ||
| }); | ||
| await goto(settingsUrl, { replaceState: true }); |
Contributor
There was a problem hiding this comment.
When a user returns from the 3DS payment flow with ?type=validate-addon in the URL but no valid addonId can be resolved (either missing from the URL or no pending BAA addon exists), the code silently redirects to settings without showing any notification. The user has no indication of whether their BAA setup succeeded or failed.
Consider adding an error notification when addonId is null:
Suggested change
| if (addonId) { | |
| try { | |
| await sdk.forConsole.organizations.validateAddonPayment({ | |
| organizationId: $organization.$id, | |
| addonId | |
| }); | |
| await Promise.all([ | |
| invalidate(Dependencies.ADDONS), | |
| invalidate(Dependencies.ORGANIZATION) | |
| ]); | |
| addNotification({ | |
| message: 'BAA addon has been enabled', | |
| type: 'success' | |
| }); | |
| } catch (e) { | |
| // If addon not found, payment webhook may have already activated it | |
| if (e?.type === 'addon_not_found' || e?.code === 404) { | |
| await Promise.all([ | |
| invalidate(Dependencies.ADDONS), | |
| invalidate(Dependencies.ORGANIZATION) | |
| ]); | |
| addNotification({ | |
| message: 'BAA addon has been enabled', | |
| type: 'success' | |
| }); | |
| } else { | |
| addNotification({ | |
| message: e.message, | |
| type: 'error' | |
| }); | |
| } | |
| } | |
| } | |
| const settingsUrl = resolve('/(console)/organization-[organization]/settings', { | |
| organization: $organization.$id | |
| }); | |
| await goto(settingsUrl, { replaceState: true }); | |
| if (addonId) { | |
| try { | |
| await sdk.forConsole.organizations.validateAddonPayment({ | |
| organizationId: $organization.$id, | |
| addonId | |
| }); | |
| await Promise.all([ | |
| invalidate(Dependencies.ADDONS), | |
| invalidate(Dependencies.ORGANIZATION) | |
| ]); | |
| addNotification({ | |
| message: 'BAA addon has been enabled', | |
| type: 'success' | |
| }); | |
| } catch (e) { | |
| // If addon not found, payment webhook may have already activated it | |
| if (e?.type === 'addon_not_found' || e?.code === 404) { | |
| await Promise.all([ | |
| invalidate(Dependencies.ADDONS), | |
| invalidate(Dependencies.ORGANIZATION) | |
| ]); | |
| addNotification({ | |
| message: 'BAA addon has been enabled', | |
| type: 'success' | |
| }); | |
| } else { | |
| addNotification({ | |
| message: e.message, | |
| type: 'error' | |
| }); | |
| } | |
| } | |
| } else { | |
| addNotification({ | |
| message: 'Could not verify BAA payment. Please check your BAA status or contact support.', | |
| type: 'error' | |
| }); | |
| } |
Comment on lines
+116
to
+129
| <div class="price-breakdown u-margin-block-start-24"> | ||
| <div class="price-row"> | ||
| <span class="text">HIPAA BAA</span> | ||
| <span class="text">{formatCurrency(BAA_MONTHLY_PRICE)} / month</span> | ||
| </div> | ||
| <hr class="divider" /> | ||
| <div class="price-row u-bold"> | ||
| <span class="text">Total</span> | ||
| <span class="text">{formatCurrency(BAA_MONTHLY_PRICE)} / month</span> | ||
| </div> | ||
| <p class="text u-color-text-offline u-margin-block-start-8 u-text-end"> | ||
| * Plus applicable tax and fees | ||
| </p> | ||
| </div> |
Contributor
There was a problem hiding this comment.
The price breakdown's "Total" row displays the full monthly price ($350) instead of the actual prorated amount the user will be charged immediately. This inconsistency is problematic for a legally-binding agreement like a BAA, where users need to see the exact charge they're authorizing.
The introductory paragraph correctly shows the prorated amount (line 107: {formatCurrency(proratedAmount)} immediately), but the breakdown table contradicts this by showing the monthly rate. A user who references the breakdown to verify their charge will see a different (higher) figure.
The "Total" row should display the prorated amount:
Suggested change
| <div class="price-breakdown u-margin-block-start-24"> | |
| <div class="price-row"> | |
| <span class="text">HIPAA BAA</span> | |
| <span class="text">{formatCurrency(BAA_MONTHLY_PRICE)} / month</span> | |
| </div> | |
| <hr class="divider" /> | |
| <div class="price-row u-bold"> | |
| <span class="text">Total</span> | |
| <span class="text">{formatCurrency(BAA_MONTHLY_PRICE)} / month</span> | |
| </div> | |
| <p class="text u-color-text-offline u-margin-block-start-8 u-text-end"> | |
| * Plus applicable tax and fees | |
| </p> | |
| </div> | |
| <div class="price-breakdown u-margin-block-start-24"> | |
| <div class="price-row"> | |
| <span class="text">HIPAA BAA</span> | |
| <span class="text">{formatCurrency(BAA_MONTHLY_PRICE)} / month</span> | |
| </div> | |
| <hr class="divider" /> | |
| <div class="price-row u-bold"> | |
| <span class="text">Total</span> | |
| <span class="text">{formatCurrency(proratedAmount)} immediately</span> | |
| </div> |
Comment on lines
+84
to
+95
| } catch (e) { | ||
| // 409 means addon already exists (pending or active from prior attempt) | ||
| if (e?.code === 409) { | ||
| await Promise.all([ | ||
| invalidate(Dependencies.ADDONS), | ||
| invalidate(Dependencies.ORGANIZATION) | ||
| ]); | ||
| show = false; | ||
| } else { | ||
| error = e.message; | ||
| trackError(e, Submit.BAAAddonEnable); | ||
| } |
Contributor
There was a problem hiding this comment.
When createBaaAddon returns a 409 (addon already exists/pending), the modal closes silently without any notification. The user has no feedback about what happened — they don't know if the addon is already active or if their request succeeded.
At minimum, a success notification should be shown to clarify that the BAA is already active or pending:
Suggested change
| } catch (e) { | |
| // 409 means addon already exists (pending or active from prior attempt) | |
| if (e?.code === 409) { | |
| await Promise.all([ | |
| invalidate(Dependencies.ADDONS), | |
| invalidate(Dependencies.ORGANIZATION) | |
| ]); | |
| show = false; | |
| } else { | |
| error = e.message; | |
| trackError(e, Submit.BAAAddonEnable); | |
| } | |
| } catch (e) { | |
| // 409 means addon already exists (pending or active from prior attempt) | |
| if (e?.code === 409) { | |
| await Promise.all([ | |
| invalidate(Dependencies.ADDONS), | |
| invalidate(Dependencies.ORGANIZATION) | |
| ]); | |
| addNotification({ | |
| message: 'BAA addon is already active or pending', | |
| type: 'success' | |
| }); | |
| show = false; | |
| } else { | |
| error = e.message; | |
| trackError(e, Submit.BAAAddonEnable); | |
| } |
Comment on lines
+64
to
+89
| async function handleReEnable() { | ||
| reEnabling = true; | ||
| try { | ||
| await sdk.forConsole.organizations.createBaaAddon({ | ||
| organizationId: $organization.$id, | ||
| key: 'baa' | ||
| }); | ||
| await Promise.all([ | ||
| invalidate(Dependencies.ADDONS), | ||
| invalidate(Dependencies.ORGANIZATION) | ||
| ]); | ||
| addNotification({ | ||
| message: 'BAA addon has been re-enabled', | ||
| type: 'success' | ||
| }); | ||
| trackEvent(Submit.BAAAddonEnable); | ||
| } catch (e) { | ||
| addNotification({ | ||
| message: e.message, | ||
| type: 'error' | ||
| }); | ||
| trackError(e, Submit.BAAAddonEnable); | ||
| } finally { | ||
| reEnabling = false; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
createBaaAddon can return either a Models.Addon (direct charge) or a Models.PaymentAuthentication object (when 3DS payment confirmation is required). The handleReEnable function discards the return value entirely and immediately shows a "BAA addon has been re-enabled" success notification.
By contrast, BAAEnableModal.svelte's handleSubmit correctly guards against this with an if ('clientSecret' in result) check before proceeding (line 60).
If createBaaAddon returns a PaymentAuthentication object during re-enable, the addon will be left in a pending state while the user believes it has been fully re-enabled — the Stripe payment confirmation step is never initiated and the charge never completes.
The handleReEnable function should be updated to:
- Capture the return value from
createBaaAddon - Check if it contains a payment authentication object (like
handleSubmitdoes at line 60) - If payment authentication is required, call
confirmPayment()with the appropriate parameters - Only show the success notification after the payment is confirmed, or if a direct charge succeeded
Refer to BAAEnableModal.svelte lines 50-72 for the correct pattern to follow.
Comment on lines
+140
to
+164
| BAA is enabled for your organization at $350/month. | ||
| </p> | ||
| {#if isScheduledForRemoval} | ||
| <p class="text u-margin-block-start-8"> | ||
| BAA will be removed at the end of your current billing cycle. | ||
| </p> | ||
| <Button | ||
| secondary | ||
| class="u-margin-block-start-16" | ||
| disabled={reEnabling} | ||
| on:click={handleReEnable}> | ||
| <span class="text">Keep BAA</span> | ||
| </Button> | ||
| {:else} | ||
| <Button | ||
| secondary | ||
| class="u-margin-block-start-16" | ||
| on:click={() => (showDisable = true)}> | ||
| <span class="text">Disable BAA</span> | ||
| </Button> | ||
| {/if} | ||
| {:else} | ||
| <p class="text u-margin-block-start-8"> | ||
| Enable BAA for your organization to ensure HIPAA compliance. This addon costs | ||
| $350/month, prorated for your current billing cycle. |
Contributor
There was a problem hiding this comment.
Hard-coded price not in sync with BAA_MONTHLY_PRICE constant
The price string $350/month is hard-coded at lines 140 and 164, but the canonical value is BAA_MONTHLY_PRICE = 350 defined only inside BAAEnableModal.svelte as a file-scoped constant. If the price is ever updated, BAAEnableModal.svelte would be changed but these two strings in BAA.svelte would be missed, causing the settings card and the enable modal to show different prices to the user.
The constant should be moved to a shared location (e.g. src/lib/constants.ts) and imported by both files. This ensures a single source of truth for billing amounts.
Comment on lines
+50
to
+99
| async function handleSubmit() { | ||
| submitting = true; | ||
| error = null; | ||
| try { | ||
| const result: Models.Addon | Models.PaymentAuthentication = | ||
| await sdk.forConsole.organizations.createBaaAddon({ | ||
| organizationId: $organization.$id, | ||
| key: 'baa' | ||
| }); | ||
|
|
||
| if ('clientSecret' in result) { | ||
| const paymentAuth = result as unknown as Models.PaymentAuthentication; | ||
| const settingsUrl = resolve('/(console)/organization-[organization]/settings', { | ||
| organization: $organization.$id | ||
| }); | ||
| await confirmPayment({ | ||
| clientSecret: paymentAuth.clientSecret, | ||
| paymentMethodId: $organization.paymentMethodId, | ||
| orgId: $organization.$id, | ||
| route: `${settingsUrl}?type=validate-addon&addonId=${paymentAuth.addonId}` | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| await Promise.all([ | ||
| invalidate(Dependencies.ADDONS), | ||
| invalidate(Dependencies.ORGANIZATION) | ||
| ]); | ||
| addNotification({ | ||
| message: 'BAA addon has been enabled', | ||
| type: 'success' | ||
| }); | ||
| trackEvent(Submit.BAAAddonEnable); | ||
| show = false; | ||
| } catch (e) { | ||
| // 409 means addon already exists (pending or active from prior attempt) | ||
| if (e?.code === 409) { | ||
| await Promise.all([ | ||
| invalidate(Dependencies.ADDONS), | ||
| invalidate(Dependencies.ORGANIZATION) | ||
| ]); | ||
| show = false; | ||
| } else { | ||
| error = e.message; | ||
| trackError(e, Submit.BAAAddonEnable); | ||
| } | ||
| } finally { | ||
| submitting = false; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Stripe confirmPayment errors misattributed in analytics as addon-creation failures
When confirmPayment (line 65) throws — e.g., due to Stripe.js initialization failure, a declined payment method during 3DS setup, or a network error — execution falls through to the outer catch block (line 84). At that point createBaaAddon has already returned successfully (the addon exists), but the error is tracked as trackError(e, Submit.BAAAddonEnable) — attributing a Stripe client-side failure to the server-side addon creation event.
This means analytics will record spurious BAAAddonEnable errors whose real cause is a Stripe configuration or network issue, making it harder to diagnose actual addon-creation failures.
Consider wrapping the confirmPayment call in its own try/catch block so Stripe errors can be tracked separately with proper context.
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.
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
Refactor
Chores