feat: pagination on billing ui table and refactoring it #2350
Conversation
ConsoleProject ID: Sites (2)
Note Appwrite has a Discord community with over 16 000 members. |
WalkthroughAdds pagination to billing aggregation and wires it through the console billing page and UI. Billing.getAggregation now accepts optional limit and offset query params. The +page.ts load function is made route/url-aware to compute page, limit, and offset, declares a BILLING_AGGREGATION dependency, and returns limit/offset with the aggregation payload. The billing PlanSummary component was refactored to an accordion-based layout, now receives pagination props (limit, offset) from the page and supports client-side project pagination and badge/display changes. package.json updates two pink-svelte package sources and a new Dependencies enum member BILLING_AGGREGATION was added. Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/routes/(console)/organization-[organization]/billing/planSummary.svelte (3)
341-346: NaN bug when availableCredit is undefinedMath.min with undefined yields NaN, propagating to totalAmount.
-$: totalAmount = Math.max(currentAggregation?.amount - creditsApplied, 0); - -$: creditsApplied = Math.min( - currentAggregation?.amount ?? currentPlan?.price ?? 0, - availableCredit -); +$: creditsApplied = Math.min( + currentAggregation?.amount ?? currentPlan?.price ?? 0, + availableCredit ?? 0 +); +$: totalAmount = Math.max((currentAggregation?.amount ?? 0) - creditsApplied, 0);
218-316: Possible runtime errors: missing optional chaining on project resource fieldsproject.* lookups (find) can be undefined; direct .value/.amount access will throw.
-usage: `${formatBandwidthUsage(project.bandwidth.value, currentPlan?.bandwidth)}`, -price: formatCurrency(project.bandwidth.amount || 0) +usage: `${formatBandwidthUsage(project.bandwidth?.value ?? 0, currentPlan?.bandwidth)}`, +price: formatCurrency(project.bandwidth?.amount ?? 0) ... -usage: `${formatNum(project.users.value || 0)} / ${currentPlan?.users ? formatNum(currentPlan.users) : 'Unlimited'}`, -price: formatCurrency(project.users.amount || 0) +usage: `${formatNum(project.users?.value ?? 0)} / ${currentPlan?.users ? formatNum(currentPlan.users) : 'Unlimited'}`, +price: formatCurrency(project.users?.amount ?? 0) ... -usage: `${formatNum(project.databasesReads.value || 0)} / ${currentPlan?.databasesReads ? formatNum(currentPlan.databasesReads) : 'Unlimited'}`, -price: formatCurrency(project.databasesReads.amount || 0) +usage: `${formatNum(project.databasesReads?.value ?? 0)} / ${currentPlan?.databasesReads ? formatNum(currentPlan.databasesReads) : 'Unlimited'}`, +price: formatCurrency(project.databasesReads?.amount ?? 0) ... -usage: `${formatNum(project.databasesWrites.value || 0)} / ${currentPlan?.databasesWrites ? formatNum(currentPlan.databasesWrites) : 'Unlimited'}`, -price: formatCurrency(project.databasesWrites.amount || 0) +usage: `${formatNum(project.databasesWrites?.value ?? 0)} / ${currentPlan?.databasesWrites ? formatNum(currentPlan.databasesWrites) : 'Unlimited'}`, +price: formatCurrency(project.databasesWrites?.amount ?? 0) ... -usage: `${formatNum(project.executions.value || 0)} / ${currentPlan?.executions ? formatNum(currentPlan.executions) : 'Unlimited'}`, -price: formatCurrency(project.executions.amount || 0) +usage: `${formatNum(project.executions?.value ?? 0)} / ${currentPlan?.executions ? formatNum(currentPlan.executions) : 'Unlimited'}`, +price: formatCurrency(project.executions?.amount ?? 0) ... -usage: `${formatHumanSize(project.storage.value || 0)} / ${currentPlan?.storage?.toString() || '0'} GB`, -price: formatCurrency(project.storage.amount || 0) +usage: `${formatHumanSize(project.storage?.value ?? 0)} / ${currentPlan?.storage?.toString() || '0'} GB`, +price: formatCurrency(project.storage?.amount ?? 0) ... -usage: `${formatNum(project.imageTransformations.value || 0)} / ${currentPlan?.imageTransformations ? formatNum(currentPlan.imageTransformations) : 'Unlimited'}`, -price: formatCurrency(project.imageTransformations.amount || 0) +usage: `${formatNum(project.imageTransformations?.value ?? 0)} / ${currentPlan?.imageTransformations ? formatNum(currentPlan.imageTransformations) : 'Unlimited'}`, +price: formatCurrency(project.imageTransformations?.amount ?? 0) ... -usage: `${formatNum(project.gbHours.value || 0)} / ${currentPlan?.GBHours ? formatNum(currentPlan.GBHours) : 'Unlimited'}`, -price: formatCurrency(project.gbHours.amount || 0) +usage: `${formatNum(project.gbHours?.value ?? 0)} / ${currentPlan?.GBHours ? formatNum(currentPlan.GBHours) : 'Unlimited'}`, +price: formatCurrency(project.gbHours?.amount ?? 0) ... -usage: `${formatNum(project.authPhone.value || 0)} SMS messages`, -price: formatCurrency(project.authPhone.amount || 0) +usage: `${formatNum(project.authPhone?.value ?? 0)} SMS messages`, +price: formatCurrency(project.authPhone?.amount ?? 0)Also update related progressData inputs to use ?.value ?? 0.
326-333: Avoid {@html} for links (XSS risk) and honor base pathBuild the link via Svelte markup to prevent injection; also prefix with base for subpath deployments.
-{ - id: `usage-details`, - cells: { - item: `<a href="/console/project-${String(project.region || 'default')}-${project.projectId}/settings/usage" style="text-decoration: underline; color: var(--fgcolor-accent-neutral);">Usage details</a>`, - usage: '', - price: '' - } -} +{ + id: `usage-details`, + cells: { item: 'Usage details', usage: '', price: '' }, + linkHref: `${base}/console/project-${String(project.region || 'default')}-${project.projectId}/settings/usage` +}-{#if child.cells?.[col.id]?.includes('<a href=')} - {@html child.cells?.[col.id] ?? ''} +{#if col.id === 'item' && child.linkHref} + <a href={child.linkHref} rel="noopener noreferrer" class="u-underline" style="color: var(--fgcolor-accent-neutral);"> + {child.cells?.[col.id]} + </a>Also applies to: 434-436
src/routes/(console)/organization-[organization]/billing/+page.svelte (1)
110-113: Fix user-facing grammar.“In organization's your projects” is incorrect.
Apply:
- To avoid service disruptions in organization's your projects, please verify your - payment details and update them if necessary. + To avoid service disruptions in your organization's projects, please verify your + payment details and update them if necessary.
🧹 Nitpick comments (10)
src/lib/sdk/billing.ts (2)
937-950: Harden limit/offset handling (avoid NaN/negatives) and send intsGuard against NaN and negative values; truncate to integers before sending.
- if (typeof limit === 'number') params['limit'] = limit; - if (typeof offset === 'number') params['offset'] = offset; + if (Number.isFinite(limit) && (limit as number) > 0) { + params['limit'] = Math.trunc(limit as number); + } + if (Number.isFinite(offset) && (offset as number) >= 0) { + params['offset'] = Math.trunc(offset as number); + }
937-942: Return type may need optional totals for paginationThe UI reads breakdownTotal/projectsTotal for pagination. Consider extending AggregationTeam with optional totals to reflect backend shape.
Would you like me to patch the type to:
export type AggregationTeam = { /* existing */ } & { breakdownTotal?: number; projectsTotal?: number; };and wire tests accordingly?
src/routes/(console)/organization-[organization]/billing/planSummary.svelte (6)
154-159: Clamp URL-driven pagination to safe boundsPrevent negative/invalid values from URL.
-$: projectsLimit = limit ?? (Number(page.url.searchParams.get('limit')) || 5); -$: projectsOffset = - offset ?? ((Number(page.url.searchParams.get('page')) || 1) - 1) * projectsLimit; +$: projectsLimit = limit ?? Math.max(1, Number(page.url.searchParams.get('limit')) || 5); +$: projectsOffset = + offset ?? + ((Math.max(1, Number(page.url.searchParams.get('page')) || 1) - 1) * projectsLimit);
781-785: Selector likely mismatched with row idCSS targets [data-expandable-row-id='additional-projects'] but row id is "addon-projects".
-:global([data-expandable-row-id='additional-projects']), +:global([data-expandable-row-id='addon-projects']), :global([data-expandable-row-id^='addon-']) {
798-804: Fix typo in commentMinor nit.
-/* reducingh size of paginator */ +/* reduce size of paginator */
1-47: Optional: tighten utility function guardsformatHumanSize/formatBandwidthUsage handle falsy values; consider coalescing inputs to 0 explicitly for consistency.
167-187: Null-safety for addons mapcurrentPlan.addons is assumed defined when filtering. Consider local var to avoid accidental undefined access.
-const addons = (currentAggregation?.resources || []) +const planAddons = currentPlan?.addons ?? {}; +const addons = (currentAggregation?.resources ?? []) .filter( (r) => r.amount && r.amount > 0 && - Object.keys(currentPlan?.addons || {}).includes(r.resourceId) && - currentPlan.addons[r.resourceId]?.price > 0 + Object.prototype.hasOwnProperty.call(planAddons, r.resourceId) && + planAddons[r.resourceId]?.price > 0 )
31-35: Prop defaults: consider number defaults for pagination inputsDefaulting limit/offset to numbers (0/undefined mix) causes conditional checks later. Prefer numeric defaults upstream to simplify.
src/routes/(console)/organization-[organization]/billing/+page.ts (2)
23-25: Optional: depends key may be unnecessary for query param nav.SvelteKit re-runs load on search param changes. Keep depends('billing:aggregation') only if you plan to invalidate it elsewhere.
95-101: Avoid recomputing pagination; reuse computed values.Prevents drift and keeps a single source of truth.
Apply:
- // expose pagination for components - limit: getLimit(url, route, 5), - offset: pageToOffset(getPage(url) || 1, getLimit(url, route, 5)), - // unique key to force component refresh on page change - aggregationKey: `agg:${getPage(url) || 1}:${getLimit(url, route, 5)}` + // expose pagination for components + limit, + offset, + // unique key to force component refresh on page change + aggregationKey: `agg:${currentPage}:${limit}`
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
package.json(1 hunks)src/lib/sdk/billing.ts(1 hunks)src/routes/(console)/organization-[organization]/billing/+page.svelte(1 hunks)src/routes/(console)/organization-[organization]/billing/+page.ts(4 hunks)src/routes/(console)/organization-[organization]/billing/planSummary.svelte(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/routes/(console)/organization-[organization]/billing/+page.ts (3)
src/routes/(console)/organization-[organization]/+page.ts (1)
load(8-42)src/lib/helpers/load.ts (3)
getPage(8-10)getLimit(12-16)pageToOffset(4-6)src/lib/stores/organization.ts (2)
organization(62-62)Organization(16-38)
🪛 ESLint
src/routes/(console)/organization-[organization]/billing/planSummary.svelte
[error] 160-160: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 161-161: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e
🔇 Additional comments (9)
package.json (1)
25-25: pnpm-lock update verified; confirm CI artifact resolution
- pnpm-lock.yaml includes the new
@appwrite.io/console@2515entry.- Verify your CI pipeline can authenticate to pkg.pr.new, resolve and cache this PR artifact URL reliably.
src/routes/(console)/organization-[organization]/billing/planSummary.svelte (5)
196-203: Nice: badge for “Additional projects”Clean UX improvement; keeps item text concise and surfaces the count visually.
350-365: Good use of keyed block to force re-render on page changes{#key aggregationKey} ensures a clean remount when pagination changes.
556-575: Analytics: good switch to typed constantUsing Click.OrganizationClickUpgrade improves event hygiene.
152-153: Incorrect page store importThe page store should come from $app/stores. Using $app/state will fail.
-import { page } from '$app/state'; +import { page } from '$app/stores';Likely an incorrect or invalid review comment.
494-503: No changes needed: PaginationComponent correctly updates thepagequery-param
PaginationComponent’sgetLinksets/removes thepageparameter on its generated links, ensuring SvelteKit navigation triggers the +page.ts loader and re-runsdepends('billing:aggregation').src/routes/(console)/organization-[organization]/billing/+page.svelte (2)
136-139: Prop pass-through for backend pagination looks correct.Passing limit, offset, and aggregationKey to PlanSummary aligns with the new load API.
136-139: Verify PlanSummary exports and wrap in key block
Ensure the PlanSummary component exportslimit,offset, andaggregationKey, and in src/routes/(console)/organization-[organization]/billing/+page.svelte wrap the<PlanSummary>invocation in{#key data.aggregationKey} <PlanSummary … /> {/key}to force a remount when the key changes.
src/routes/(console)/organization-[organization]/billing/+page.ts (1)
9-10: Imports for pagination helpers look good.
| (currentAggregation && (currentAggregation as any).breakdownTotal) || | ||
| (currentAggregation && (currentAggregation as any).projectsTotal) || | ||
| (currentAggregation?.resources?.find?.((r) => r.resourceId === 'projects')?.value ?? | ||
| null) || | ||
| currentAggregation?.breakdown?.length || | ||
| 0; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix ESLint “no-explicit-any” and keep strong typing for totals
Avoid casts to any by introducing a local augmentation type.
+$: type AggregationTotals = Partial<{ breakdownTotal: number; projectsTotal: number }>;
$: totalProjects =
- (currentAggregation && (currentAggregation as any).breakdownTotal) ||
- (currentAggregation && (currentAggregation as any).projectsTotal) ||
+ (currentAggregation && (currentAggregation as AggregationTotals).breakdownTotal) ||
+ (currentAggregation && (currentAggregation as AggregationTotals).projectsTotal) ||
(currentAggregation?.resources?.find?.((r) => r.resourceId === 'projects')?.value ??
null) ||
currentAggregation?.breakdown?.length ||
0;Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 ESLint
[error] 160-160: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 161-161: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
🤖 Prompt for AI Agents
In src/routes/(console)/organization-[organization]/billing/planSummary.svelte
around lines 160 to 165, avoid the explicit any casts by introducing a local
typed interface/alias that augments the shape of currentAggregation (e.g.,
fields breakdownTotal?: number, projectsTotal?: number, resources?: {
resourceId: string; value?: number }[], breakdown?: unknown[]), annotate
currentAggregation with that type, and then rewrite the expression to use the
typed properties with optional chaining and nullish coalescing (no as any casts)
so the totals resolution remains the same but with strong typing.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/routes/(console)/organization-[organization]/billing/planSummary.svelte (2)
341-347: Avoid NaN flicker and undefined credit: compute credits first and default to 0
totalAmountusescreditsAppliedbefore it’s defined, andMath.min(..., availableCredit)yieldsNaNwhenavailableCreditis undefined.Apply:
-$: totalAmount = Math.max(currentAggregation?.amount - creditsApplied, 0); - -$: creditsApplied = Math.min( - currentAggregation?.amount ?? currentPlan?.price ?? 0, - availableCredit -); +$: creditsApplied = Math.min( + (currentAggregation?.amount ?? currentPlan?.price ?? 0), + (availableCredit ?? 0) +); +$: totalAmount = Math.max((currentAggregation?.amount ?? 0) - creditsApplied, 0);
205-334: Guard resource access with optional chaining to prevent crashesSeveral
project.<resource>.value/amountreads assume the resource exists. When absent, this dereferencesundefinedand crashes the UI.Apply the pattern
project.<res>?.value ?? 0andproject.<res>?.amount ?? 0. For example:-usage: `${formatBandwidthUsage(project.bandwidth.value, currentPlan?.bandwidth)}`, -price: formatCurrency(project.bandwidth.amount || 0) +usage: `${formatBandwidthUsage(project.bandwidth?.value ?? 0, currentPlan?.bandwidth)}`, +price: formatCurrency(project.bandwidth?.amount ?? 0) ... -usage: `${formatNum(project.users.value || 0)} / ${currentPlan?.users ? ...}`, -price: formatCurrency(project.users.amount || 0) +usage: `${formatNum(project.users?.value ?? 0)} / ${currentPlan?.users ? ...}`, +price: formatCurrency(project.users?.amount ?? 0) ... -usage: `${formatNum(project.databasesReads.value || 0)} / ...`, -price: formatCurrency(project.databasesReads.amount || 0) +usage: `${formatNum(project.databasesReads?.value ?? 0)} / ...`, +price: formatCurrency(project.databasesReads?.amount ?? 0) ... -usage: `${formatNum(project.databasesWrites.value || 0)} / ...`, -price: formatCurrency(project.databasesWrites.amount || 0) +usage: `${formatNum(project.databasesWrites?.value ?? 0)} / ...`, +price: formatCurrency(project.databasesWrites?.amount ?? 0) ... -usage: `${formatNum(project.executions.value || 0)} / ...`, -price: formatCurrency(project.executions.amount || 0) +usage: `${formatNum(project.executions?.value ?? 0)} / ...`, +price: formatCurrency(project.executions?.amount ?? 0) ... -usage: `${formatHumanSize(project.storage.value || 0)} / ...`, -price: formatCurrency(project.storage.amount || 0) +usage: `${formatHumanSize(project.storage?.value ?? 0)} / ...`, +price: formatCurrency(project.storage?.amount ?? 0) ... -usage: `${formatNum(project.imageTransformations.value || 0)} / ...`, -price: formatCurrency(project.imageTransformations.amount || 0) +usage: `${formatNum(project.imageTransformations?.value ?? 0)} / ...`, +price: formatCurrency(project.imageTransformations?.amount ?? 0) ... -usage: `${formatNum(project.gbHours.value || 0)} / ...`, +usage: `${formatNum(project.gbHours?.value ?? 0)} / ...`, ... - ? createProgressData(project.gbHours.value || 0, currentPlan.GBHours) + ? createProgressData(project.gbHours?.value ?? 0, currentPlan.GBHours) ... -usage: `${formatNum(project.authPhone.value || 0)} SMS messages`, -price: formatCurrency(project.authPhone.amount || 0) +usage: `${formatNum(project.authPhone?.value ?? 0)} SMS messages`, +price: formatCurrency(project.authPhone?.amount ?? 0)Also update progress inputs similarly:
-progressData: createStorageProgressData(project.bandwidth.value || 0, ...), +progressData: createStorageProgressData(project.bandwidth?.value ?? 0, ...),
♻️ Duplicate comments (1)
src/routes/(console)/organization-[organization]/billing/planSummary.svelte (1)
159-165: Use nullish coalescing and proper typing; dropanycasts
||will skip legitimate zeros;as anyviolates lint rules. Type narrow and use??.Apply:
+$: type AggregationTotals = Partial<{ breakdownTotal: number; projectsTotal: number }>; $: totalProjects = - (currentAggregation && (currentAggregation as any).breakdownTotal) || - (currentAggregation && (currentAggregation as any).projectsTotal) || - (currentAggregation?.resources?.find?.((r) => r.resourceId === 'projects')?.value ?? - null) || - currentAggregation?.breakdown?.length || - 0; + (currentAggregation as AggregationTotals)?.breakdownTotal ?? + (currentAggregation as AggregationTotals)?.projectsTotal ?? + currentAggregation?.resources?.find?.((r) => r.resourceId === 'projects')?.value ?? + currentAggregation?.breakdown?.length ?? + 0;
🧹 Nitpick comments (10)
package.json (1)
25-29: Pin PR/VC tarball deps or swap to stable tags before mergeUsing URL tarballs from pkg.pr.new/pkg.vc is fine for iterating, but it can hurt reproducibility and break in the future. Either:
- keep them commit/immutable and ensure CI caches/allowlists them, or
- switch to published, semver-pinned releases before merging to main.
Also verify the lockfile captures integrity for these URLs.
src/routes/(console)/organization-[organization]/billing/planSummary.svelte (9)
326-333: Avoid injecting HTML; render an element and includebaseString HTML +
{@html}is unnecessary and bypasses type checks. Also, prefix withbasefor subpath deployments.Apply:
- item: `<a href="/console/project-${String(project.region || 'default')}-${project.projectId}/settings/usage" style="text-decoration: underline; color: var(--fgcolor-accent-neutral);">Usage details</a>`, + item: { + type: 'link', + href: `${base}/console/project-${String(project.region || 'default')}-${project.projectId}/settings/usage`, + label: 'Usage details' + },And in the summary cell:
-{#if child.cells?.item?.includes('<a href=')} - {@html child.cells?.item ?? ''} -{:else} +{#if typeof child.cells?.item === 'object' && child.cells?.item?.type === 'link'} + <a href={child.cells.item.href} class="usage-link"> {child.cells.item.label} </a> +{:else}
187-203: Badge logic: handle zero count explicitly
row.badgeis falsy for zero and won’t render. If zero should be shown, coerce to string and check!== null && !== undefined.-badge: excess.resourceId === 'projects' ? formatNum(excess.value) : null, +badge: excess.resourceId === 'projects' ? String(formatNum(excess.value ?? 0)) : null,And:
-{#if row.badge} +{#if row.badge !== null && row.badge !== undefined}
742-750: Dead selector? Align CSS with runtime IDsSelector targets
[data-expandable-row-id='additional-projects']but no row uses that ID; addons useid: addon-.... Either remove or update to the actual attribute used byAccordionTable.Row.
763-769: Typo and styling nitFix comment typo and consider using a tokenized size instead of scale transform (better for a11y and crispness).
-/* reducingh size of paginator */ +/* reducing size of paginator */
38-43: Columns widths: guard against narrow viewportsStatic
minwidths of 500 + 200 can overflow on small screens. Consider responsive widths or CSS clamp.
118-150: TypecurrentAggregationand return shape ofgetProjectsListAdd types to eliminate implicit any and catch resource ID typos at compile time.
-function getProjectsList(currentAggregation) { +type ProjectResource = { resourceId: string; value?: number; amount?: number }; +type ProjectBreakdown = { + $id: string; name?: string; region?: string; amount?: number; resources?: ProjectResource[]; +}; +function getProjectsList(currentAggregation: { breakdown?: ProjectBreakdown[] } | undefined) {
493-506: Pagination row condition: prefer ≥ and null-safe sumsIf
totalProjects === projectsLimit, consider whether to show pagination for page=1. Also ensuresumis a number.-{#if totalProjects > projectsLimit} +{#if (Number(totalProjects) || 0) >= (Number(projectsLimit) || 0)}
507-531: Credits row visibility matchescreditsAppliednotavailableCreditCurrently hidden when
availableCreditis unset but computed credits > 0 (after defaulting to 0). Show row based oncreditsApplied > 0.-{#if availableCredit > 0} +{#if creditsApplied > 0}
518-529: Currency on credits: ensure negative formatting onceYou already prefix
-in template; ensureformatCurrencydoesn’t double-negative on negative values. Safer to pass positive and keep the-in template, or pass-creditsAppliedand drop the manual-.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
package.json(1 hunks)src/routes/(console)/organization-[organization]/billing/planSummary.svelte(7 hunks)
🧰 Additional context used
🪛 ESLint
src/routes/(console)/organization-[organization]/billing/planSummary.svelte
[error] 160-160: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 161-161: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
🔇 Additional comments (4)
src/routes/(console)/organization-[organization]/billing/planSummary.svelte (4)
349-365: Keyed re-render: good use of {#key} for pagination reloadsThis prevents stale rows on page changes and matches the loader’s
aggregationKey.Ensure loaders update
aggregationKeywheneverpageorlimitchanges so this reliably remounts.
554-573: Analytics event: good switch to typed enumConsistent with the rest of the codebase and safer than string literals.
320-336: Units and max value for bandwidth/storage
formatBandwidthUsagetreats max as GB; progress bars setmaxValuein bytes. Confirm resource units are consistent across usage and progress.
1-23: Imports look coherent; keep components sourced from one bundleNice consolidation of UI imports through pink-svelte and $lib/components.
| import { page } from '$app/state'; | ||
| let projectsLimit: number = 5; | ||
| let projectsOffset: number = 0; | ||
| $: projectsLimit = limit ?? (Number(page.url.searchParams.get('limit')) || 5); | ||
| $: projectsOffset = | ||
| offset ?? ((Number(page.url.searchParams.get('page')) || 1) - 1) * projectsLimit; |
There was a problem hiding this comment.
Fix store usage: use $page (value) not page (store object)
Accessing page.url reads the store object, not its value, causing runtime errors. Use $page in reactive statements.
Apply:
-import { page } from '$app/state';
+import { page } from '$app/stores';
...
-$: projectsLimit = limit ?? (Number(page.url.searchParams.get('limit')) || 5);
-$: projectsOffset =
- offset ?? ((Number(page.url.searchParams.get('page')) || 1) - 1) * projectsLimit;
+$: projectsLimit = limit ?? (Number($page.url.searchParams.get('limit')) || 5);
+$: projectsOffset =
+ offset ?? ((Number($page.url.searchParams.get('page')) || 1) - 1) * projectsLimit;📝 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.
| import { page } from '$app/state'; | |
| let projectsLimit: number = 5; | |
| let projectsOffset: number = 0; | |
| $: projectsLimit = limit ?? (Number(page.url.searchParams.get('limit')) || 5); | |
| $: projectsOffset = | |
| offset ?? ((Number(page.url.searchParams.get('page')) || 1) - 1) * projectsLimit; | |
| import { page } from '$app/stores'; | |
| let projectsLimit: number = 5; | |
| let projectsOffset: number = 0; | |
| $: projectsLimit = limit ?? (Number($page.url.searchParams.get('limit')) || 5); | |
| $: projectsOffset = | |
| offset ?? ((Number($page.url.searchParams.get('page')) || 1) - 1) * projectsLimit; |
🤖 Prompt for AI Agents
In src/routes/(console)/organization-[organization]/billing/planSummary.svelte
around lines 152 to 158, the reactive statements use the store object `page`
instead of its value which causes runtime errors; update the reactive
assignments to reference the store value `$page` (e.g., use
`$page.url.searchParams.get(...)`) so the reactive expressions read the current
URL values, keeping the existing import of `page` intact and leaving the rest of
the logic unchanged.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/routes/(console)/organization-[organization]/billing/planSummary.svelte (3)
341-347: Fix NaN and dependency ordering in totals calculation.If
availableCreditisundefinedorcurrentAggregationisundefined,creditsApplied/totalAmountcan becomeNaN. Also compute base first for clarity.-$: totalAmount = Math.max(currentAggregation?.amount - creditsApplied, 0); - -$: creditsApplied = Math.min( - currentAggregation?.amount ?? currentPlan?.price ?? 0, - availableCredit -); +$: const baseAmount = (currentAggregation?.amount ?? currentPlan?.price ?? 0); +$: creditsApplied = Math.min(baseAmount, availableCredit ?? 0); +$: totalAmount = Math.max(baseAmount - creditsApplied, 0);
214-334: Guard all project resource reads with optional chaining to avoid runtime crashes.
project.<resource>.value/amountcan beundefined(resources are looked up withfind). Direct property access will throw.Apply optional chaining across the child rows (pattern shown; repeat for all similar fields):
@@ - usage: `${formatBandwidthUsage(project.bandwidth.value, currentPlan?.bandwidth)}`, - price: formatCurrency(project.bandwidth.amount || 0) + usage: `${formatBandwidthUsage(project.bandwidth?.value || 0, currentPlan?.bandwidth)}`, + price: formatCurrency(project.bandwidth?.amount || 0) @@ - progressData: createStorageProgressData( - project.bandwidth.value || 0, + progressData: createStorageProgressData( + project.bandwidth?.value || 0, currentPlan?.bandwidth || 0 ), @@ - usage: `${formatNum(project.users.value || 0)} / ${currentPlan?.users ? formatNum(currentPlan.users) : 'Unlimited'}`, - price: formatCurrency(project.users.amount || 0) + usage: `${formatNum(project.users?.value || 0)} / ${currentPlan?.users ? formatNum(currentPlan.users) : 'Unlimited'}`, + price: formatCurrency(project.users?.amount || 0) @@ - progressData: createProgressData(project.users.value || 0, currentPlan?.users), + progressData: createProgressData(project.users?.value || 0, currentPlan?.users), @@ - usage: `${formatNum(project.databasesReads.value || 0)} / ${currentPlan?.databasesReads ? formatNum(currentPlan.databasesReads) : 'Unlimited'}`, - price: formatCurrency(project.databasesReads.amount || 0) + usage: `${formatNum(project.databasesReads?.value || 0)} / ${currentPlan?.databasesReads ? formatNum(currentPlan.databasesReads) : 'Unlimited'}`, + price: formatCurrency(project.databasesReads?.amount || 0) @@ - progressData: createProgressData( - project.databasesReads.value || 0, + progressData: createProgressData( + project.databasesReads?.value || 0, currentPlan?.databasesReads ), @@ - usage: `${formatNum(project.databasesWrites.value || 0)} / ${currentPlan?.databasesWrites ? formatNum(currentPlan.databasesWrites) : 'Unlimited'}`, - price: formatCurrency(project.databasesWrites.amount || 0) + usage: `${formatNum(project.databasesWrites?.value || 0)} / ${currentPlan?.databasesWrites ? formatNum(currentPlan.databasesWrites) : 'Unlimited'}`, + price: formatCurrency(project.databasesWrites?.amount || 0) @@ - progressData: createProgressData( - project.databasesWrites.value || 0, + progressData: createProgressData( + project.databasesWrites?.value || 0, currentPlan?.databasesWrites ), @@ - usage: `${formatNum(project.executions.value || 0)} / ${currentPlan?.executions ? formatNum(currentPlan.executions) : 'Unlimited'}`, - price: formatCurrency(project.executions.amount || 0) + usage: `${formatNum(project.executions?.value || 0)} / ${currentPlan?.executions ? formatNum(currentPlan.executions) : 'Unlimited'}`, + price: formatCurrency(project.executions?.amount || 0) @@ - progressData: createProgressData( - project.executions.value || 0, + progressData: createProgressData( + project.executions?.value || 0, currentPlan?.executions ), @@ - usage: `${formatHumanSize(project.storage.value || 0)} / ${currentPlan?.storage?.toString() || '0'} GB`, - price: formatCurrency(project.storage.amount || 0) + usage: `${formatHumanSize(project.storage?.value || 0)} / ${currentPlan?.storage?.toString() || '0'} GB`, + price: formatCurrency(project.storage?.amount || 0) @@ - progressData: createStorageProgressData( - project.storage.value || 0, + progressData: createStorageProgressData( + project.storage?.value || 0, currentPlan?.storage || 0 ), @@ - usage: `${formatNum(project.imageTransformations.value || 0)} / ${currentPlan?.imageTransformations ? formatNum(currentPlan.imageTransformations) : 'Unlimited'}`, - price: formatCurrency(project.imageTransformations.amount || 0) + usage: `${formatNum(project.imageTransformations?.value || 0)} / ${currentPlan?.imageTransformations ? formatNum(currentPlan.imageTransformations) : 'Unlimited'}`, + price: formatCurrency(project.imageTransformations?.amount || 0) @@ - progressData: createProgressData( - project.imageTransformations.value || 0, + progressData: createProgressData( + project.imageTransformations?.value || 0, currentPlan?.imageTransformations ), @@ - usage: `${formatNum(project.gbHours.value || 0)} / ${currentPlan?.GBHours ? formatNum(currentPlan.GBHours) : 'Unlimited'}`, - price: formatCurrency(project.gbHours.amount || 0) + usage: `${formatNum(project.gbHours?.value || 0)} / ${currentPlan?.GBHours ? formatNum(currentPlan.GBHours) : 'Unlimited'}`, + price: formatCurrency(project.gbHours?.amount || 0) @@ - ? createProgressData(project.gbHours.value || 0, currentPlan.GBHours) + ? createProgressData(project.gbHours?.value || 0, currentPlan.GBHours) @@ - usage: `${formatNum(project.authPhone.value || 0)} SMS messages`, - price: formatCurrency(project.authPhone.amount || 0) + usage: `${formatNum(project.authPhone?.value || 0)} SMS messages`, + price: formatCurrency(project.authPhone?.amount || 0)
326-333: Avoid{@html ...}; render links safely to prevent XSS.The current approach injects raw HTML with user-derived data (
project.region,projectId). Replace with a bound<a>element.- { - id: `usage-details`, - cells: { - item: `<a href="/console/project-${String(project.region || 'default')}-${project.projectId}/settings/usage" style="text-decoration: underline; color: var(--fgcolor-accent-neutral);">Usage details</a>`, - usage: '', - price: '' - } - }, + { + id: `usage-details`, + cells: { + item: 'Usage details', + linkHref: `${base}/project-${String(project.region || 'default')}-${project.projectId}/settings/usage`, + usage: '', + price: '' + } + }, @@ - {#if child.cells?.item?.includes('<a href=')} - {@html child.cells?.item ?? ''} - {:else} - <Typography.Text - variant="m-400" - color="--fgcolor-neutral-secondary"> - {child.cells?.item ?? ''} - </Typography.Text> - {/if} + {#if child.cells?.linkHref} + <a href={child.cells.linkHref} + style="text-decoration: underline; color: var(--fgcolor-accent-neutral);"> + Usage details + </a> + {:else} + <Typography.Text variant="m-400" color="--fgcolor-neutral-secondary"> + {child.cells?.item ?? ''} + </Typography.Text> + {/if}Also applies to: 421-429
♻️ Duplicate comments (2)
src/routes/(console)/organization-[organization]/billing/planSummary.svelte (2)
152-158: Fix SvelteKit store usage: import from $app/stores and read $page (value), not the store object.Using
'$app/state'andpage.urlreads the store object and will break at runtime. Use'$app/stores'and$page.url.-import { page } from '$app/state'; +import { page } from '$app/stores'; @@ -$: projectsLimit = limit ?? (Number(page.url.searchParams.get('limit')) || 5); -$: projectsOffset = - offset ?? ((Number(page.url.searchParams.get('page')) || 1) - 1) * projectsLimit; +$: projectsLimit = limit ?? (Number($page.url.searchParams.get('limit')) || 5); +$: projectsOffset = + offset ?? ((Number($page.url.searchParams.get('page')) || 1) - 1) * projectsLimit;
160-165: Removeanyand preserve 0-values with nullish coalescing.Two issues:
as anyviolates lint rules.- Using
||drops valid 0 values; use??.Apply this diff for the reactive expression:
-$: totalProjects = - (currentAggregation && (currentAggregation as any).breakdownTotal) || - (currentAggregation && (currentAggregation as any).projectsTotal) || - (currentAggregation?.resources?.find?.((r) => r.resourceId === 'projects')?.value ?? - null) || - currentAggregation?.breakdown?.length || - 0; +$: totalProjects = + (currentAggregation as AggregationTotals)?.breakdownTotal ?? + (currentAggregation as AggregationTotals)?.projectsTotal ?? + currentAggregation?.resources?.find?.((r) => r.resourceId === 'projects')?.value ?? + currentAggregation?.breakdown?.length ?? + 0;Add this local type (outside the range, e.g., below imports):
type AggregationTotals = Partial<{ breakdownTotal: number; projectsTotal: number; resources: { resourceId: string; value?: number }[]; breakdown: unknown[]; }>;
🧹 Nitpick comments (4)
src/routes/(console)/organization-[organization]/billing/planSummary.svelte (4)
367-372: Use the existing date helper consistently and guard for missing dates.Minor polish: prefer
toLocaleDate(...)everywhere and avoidInvalid Datewhen org fields are absent.- Current billing cycle ({new Date( - $organization?.billingCurrentInvoiceDate - ).toLocaleDateString('en', { day: 'numeric', month: 'short' })}-{new Date( - $organization?.billingNextInvoiceDate - ).toLocaleDateString('en', { day: 'numeric', month: 'short' })}) + Current billing cycle ({toLocaleDate($organization?.billingCurrentInvoiceDate, 'en', { day: 'numeric', month: 'short' })}- + {toLocaleDate($organization?.billingNextInvoiceDate, 'en', { day: 'numeric', month: 'short' })})
746-751: Tidy selector: stray target foradditional-projects.There’s no row with id
additional-projects. Either drop it or target the actualaddon-projectsid (you already have the^='addon-'selector).- :global([data-expandable-row-id='additional-projects']), - :global([data-expandable-row-id^='addon-']) { + :global([data-expandable-row-id^='addon-']) { background-color: var(--bgcolor-neutral-tertiary); border-left: 3px solid var(--bgcolor-accent-neutral); }
763-768: Fix typo in comment.Micro nit.
-/* reducingh size of paginator */ +/* reduce size of paginator */
555-573: Unify analytics event naming.One button uses
Click.OrganizationClickUpgrade, the other a string literal. Prefer typed constants for both if available.- on:click={() => - trackEvent('click_organization_plan_update', { + on:click={() => + trackEvent(Click.OrganizationClickUpgrade, { from: 'button', source: 'billing_tab' })}If a dedicated event exists for “plan update,” switch to that constant instead.
Also applies to: 575-601
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/routes/(console)/organization-[organization]/billing/planSummary.svelte(7 hunks)
🧰 Additional context used
🪛 ESLint
src/routes/(console)/organization-[organization]/billing/planSummary.svelte
[error] 160-160: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 161-161: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
🔇 Additional comments (3)
src/routes/(console)/organization-[organization]/billing/planSummary.svelte (3)
195-202: Nice UX: "Additional projects" label + badge.Good call to simplify the label and surface the count via Badge.
351-353: Keyed block to force re-mount is appropriate.This will reset internal state on page changes, avoiding stale data.
497-501: Pagination updates 'page' (not 'offset') and preserves 'limit' — loader will re-runConfirmed: src/lib/components/pagination.svelte builds links from pageStore.url and sets/deletes the 'page' param (not 'offset'), preserving other query params; planSummary.svelte reads page.url.searchParams to derive projectsLimit/projectsOffset (src/routes/.../planSummary.svelte:156–158) and +page.ts uses getPage/getLimit to compute offset and depends('billing:aggregation'), so clicking the PaginationComponent updates the query string (page, with limit preserved) and triggers the loader to refresh.
src/routes/(console)/organization-[organization]/billing/planSummary.svelte
Outdated
Show resolved
Hide resolved
src/routes/(console)/organization-[organization]/billing/+page.ts
Outdated
Show resolved
Hide resolved
| limit: getLimit(url, route, 5), | ||
| offset: pageToOffset(getPage(url) || 1, getLimit(url, route, 5)), | ||
| // unique key to force component refresh on page change | ||
| aggregationKey: `agg:${getPage(url) || 1}:${getLimit(url, route, 5)}` |
There was a problem hiding this comment.
why not just invalidate the dependency?
There was a problem hiding this comment.
depends() would refresh the data but not reset the component state, so expanded rows from the previous page would still persist. Using aggregationKey forces a full component reset on pagination change, which cleanly fixes the expansion state issue.
There was a problem hiding this comment.
2025-09-27.17-31-11.mp4
When only invalidating dependency
There was a problem hiding this comment.
Need to do a hard refresh for rows to populate with data
There was a problem hiding this comment.
depends() would refresh the data but not reset the component state
then we should derive the component values via runes or use reactive vars if on svelte4.
There was a problem hiding this comment.
this still applies @HarshMN2345?
src/routes/(console)/organization-[organization]/billing/planSummary.svelte
Outdated
Show resolved
Hide resolved
src/routes/(console)/organization-[organization]/billing/planSummary.svelte
Outdated
Show resolved
Hide resolved
src/routes/(console)/organization-[organization]/billing/planSummary.svelte
Outdated
Show resolved
Hide resolved
src/routes/(console)/organization-[organization]/billing/planSummary.svelte
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/routes/(console)/organization-[organization]/billing/planSummary.svelte (3)
216-226: Prevent crashes when resource entries are missingSeveral lookups assume a resource exists (e.g., project.bandwidth.value). If undefined, this throws. Use optional chaining and default values.
-usage: `${formatBandwidthUsage(project.bandwidth.value, currentPlan?.bandwidth)}`, -price: formatCurrency(project.bandwidth.amount || 0) +usage: `${formatBandwidthUsage(project.bandwidth?.value ?? 0, currentPlan?.bandwidth)}`, +price: formatCurrency(project.bandwidth?.amount ?? 0) - progressData: createStorageProgressData( - project.bandwidth.value || 0, + progressData: createStorageProgressData( + project.bandwidth?.value ?? 0, currentPlan?.bandwidth || 0 ), -usage: `${formatNum(project.users.value || 0)} / ${currentPlan?.users ? formatNum(currentPlan.users) : 'Unlimited'}`, -price: formatCurrency(project.users.amount || 0) +usage: `${formatNum(project.users?.value ?? 0)} / ${currentPlan?.users ? formatNum(currentPlan.users) : 'Unlimited'}`, +price: formatCurrency(project.users?.amount ?? 0) - progressData: createProgressData(project.users.value || 0, currentPlan?.users), + progressData: createProgressData(project.users?.value ?? 0, currentPlan?.users), -usage: `${formatNum(project.databasesReads.value || 0)} / ${currentPlan?.databasesReads ? formatNum(currentPlan.databasesReads) : 'Unlimited'}`, -price: formatCurrency(project.databasesReads.amount || 0) +usage: `${formatNum(project.databasesReads?.value ?? 0)} / ${currentPlan?.databasesReads ? formatNum(currentPlan.databasesReads) : 'Unlimited'}`, +price: formatCurrency(project.databasesReads?.amount ?? 0) - progressData: createProgressData( - project.databasesReads.value || 0, + progressData: createProgressData( + project.databasesReads?.value ?? 0, currentPlan?.databasesReads ), -usage: `${formatNum(project.databasesWrites.value || 0)} / ${currentPlan?.databasesWrites ? formatNum(currentPlan.databasesWrites) : 'Unlimited'}`, -price: formatCurrency(project.databasesWrites.amount || 0) +usage: `${formatNum(project.databasesWrites?.value ?? 0)} / ${currentPlan?.databasesWrites ? formatNum(currentPlan.databasesWrites) : 'Unlimited'}`, +price: formatCurrency(project.databasesWrites?.amount ?? 0) - progressData: createProgressData( - project.databasesWrites.value || 0, + progressData: createProgressData( + project.databasesWrites?.value ?? 0, currentPlan?.databasesWrites ), -usage: `${formatNum(project.executions.value || 0)} / ${currentPlan?.executions ? formatNum(currentPlan.executions) : 'Unlimited'}`, -price: formatCurrency(project.executions.amount || 0) +usage: `${formatNum(project.executions?.value ?? 0)} / ${currentPlan?.executions ? formatNum(currentPlan.executions) : 'Unlimited'}`, +price: formatCurrency(project.executions?.amount ?? 0) - progressData: createProgressData( - project.executions.value || 0, + progressData: createProgressData( + project.executions?.value ?? 0, currentPlan?.executions ), -usage: `${formatHumanSize(project.storage.value || 0)} / ${currentPlan?.storage?.toString() || '0'} GB`, -price: formatCurrency(project.storage.amount || 0) +usage: `${formatHumanSize(project.storage?.value ?? 0)} / ${currentPlan?.storage?.toString() || '0'} GB`, +price: formatCurrency(project.storage?.amount ?? 0) - progressData: createStorageProgressData( - project.storage.value || 0, + progressData: createStorageProgressData( + project.storage?.value ?? 0, currentPlan?.storage || 0 ), -usage: `${formatNum(project.imageTransformations.value || 0)} / ${currentPlan?.imageTransformations ? formatNum(currentPlan.imageTransformations) : 'Unlimited'}`, -price: formatCurrency(project.imageTransformations.amount || 0) +usage: `${formatNum(project.imageTransformations?.value ?? 0)} / ${currentPlan?.imageTransformations ? formatNum(currentPlan.imageTransformations) : 'Unlimited'}`, +price: formatCurrency(project.imageTransformations?.amount ?? 0) - progressData: createProgressData( - project.imageTransformations.value || 0, + progressData: createProgressData( + project.imageTransformations?.value ?? 0, currentPlan?.imageTransformations ), -usage: `${formatNum(project.gbHours.value || 0)} / ${currentPlan?.GBHours ? formatNum(currentPlan.GBHours) : 'Unlimited'}`, -price: formatCurrency(project.gbHours.amount || 0) +usage: `${formatNum(project.gbHours?.value ?? 0)} / ${currentPlan?.GBHours ? formatNum(currentPlan.GBHours) : 'Unlimited'}`, +price: formatCurrency(project.gbHours?.amount ?? 0) - progressData: currentPlan?.GBHours - ? createProgressData(project.gbHours.value || 0, currentPlan.GBHours) + progressData: currentPlan?.GBHours + ? createProgressData(project.gbHours?.value ?? 0, currentPlan.GBHours) : [], -usage: `${formatNum(project.authPhone.value || 0)} SMS messages`, -price: formatCurrency(project.authPhone.amount || 0) +usage: `${formatNum(project.authPhone?.value ?? 0)} SMS messages`, +price: formatCurrency(project.authPhone?.amount ?? 0)Also applies to: 230-236, 240-249, 253-262, 266-275, 279-288, 292-301, 305-313, 317-321
167-175: Remove unsafe {@html}; render link safely and stop shadowingbaseUsing {@html} with dynamic data is an XSS risk. Also, local const base shadows $app/paths base. Rename row var and pass a dedicated href property.
- const base = { + const basePlanRow = { id: 'base-plan', expandable: false, cells: { item: 'Base plan', usage: '', price: formatCurrency(nextPlan?.price ?? currentPlan?.price ?? 0) }, children: [] }; ... - { - id: `usage-details`, - cells: { - item: `<a href="/console/project-${String(project.region || 'default')}-${project.projectId}/settings/usage" style="text-decoration: underline; color: var(--fgcolor-accent-neutral);">Usage details</a>`, - usage: '', - price: '' - } - } + { + id: `usage-details`, + linkHref: `${base}/project-${String(project.region || 'default')}-${project.projectId}/settings/usage`, + cells: { + item: 'Usage details', + usage: '', + price: '' + } + } ... - return [base, ...addons, ...projects, ...noProjects]; + return [basePlanRow, ...addons, ...projects, ...noProjects];- {#if child.cells?.item?.includes('<a href=')} - {@html child.cells?.item ?? ''} + {#if child.linkHref} + <a + href={child.linkHref} + style="text-decoration: underline; color: var(--fgcolor-accent-neutral);" + > + Usage details + </a> {:else} <Typography.Text variant="m-400" color="--fgcolor-neutral-secondary"> {child.cells?.item ?? ''} </Typography.Text> {/if}Also applies to: 323-329, 418-426, 333-334
338-344: Avoid NaN totals when credits are undefinedCoalesce availableCredit to 0 and base the total off a single base amount to keep numbers stable.
-$: totalAmount = Math.max(currentAggregation?.amount - creditsApplied, 0); - -$: creditsApplied = Math.min( - currentAggregation?.amount ?? currentPlan?.price ?? 0, - availableCredit -); +$: baseAmount = currentAggregation?.amount ?? currentPlan?.price ?? 0; +$: creditsApplied = Math.min(baseAmount, availableCredit ?? 0); +$: totalAmount = Math.max(baseAmount - creditsApplied, 0);
🧹 Nitpick comments (1)
src/routes/(console)/organization-[organization]/billing/planSummary.svelte (1)
768-773: Fix typo in commentMinor nit.
- /* reducingh size of paginator */ + /* reducing size of paginator */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/lib/constants.ts(1 hunks)src/routes/(console)/organization-[organization]/billing/+page.ts(4 hunks)src/routes/(console)/organization-[organization]/billing/planSummary.svelte(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/routes/(console)/organization-[organization]/billing/+page.ts
🔇 Additional comments (3)
src/routes/(console)/organization-[organization]/billing/planSummary.svelte (2)
26-26: Fix invalid SvelteKit store import and usageImport page from $app/stores and dereference the store with $page in reactive statements. Current code will error at runtime. (Previously flagged)
-import { page } from '$app/state'; +import { page } from '$app/stores'; -$: projectsLimit = limit ?? (Number(page.url.searchParams.get('limit')) || 5); -$: projectsOffset = - offset ?? ((Number(page.url.searchParams.get('page')) || 1) - 1) * projectsLimit; +$: projectsLimit = limit ?? (Number($page.url.searchParams.get('limit')) || 5); +$: projectsOffset = + offset ?? ((Number($page.url.searchParams.get('page')) || 1) - 1) * projectsLimit;Also applies to: 155-157
490-503: Confirm pagination updates URL and triggers reloadThe
PaginationComponentgenerates<a>links via SvelteKit’spagestore (getLink) and only mutates thepagequery param—clicking a page number updates the URL and remounts the{#key}block.src/lib/constants.ts (1)
23-24: Dependency key usage is correct
depends calls use Dependencies.BILLING_AGGREGATION and no hardcoded ‘billing:aggregation’ remains.
| limit: getLimit(url, route, 5), | ||
| offset: pageToOffset(getPage(url) || 1, getLimit(url, route, 5)), | ||
| // unique key to force component refresh on page change | ||
| aggregationKey: `agg:${getPage(url) || 1}:${getLimit(url, route, 5)}` |
There was a problem hiding this comment.
depends() would refresh the data but not reset the component state
then we should derive the component values via runes or use reactive vars if on svelte4.
There was a problem hiding this comment.
lets move this to Svelte5 as well. using $derived might also help us get rid of the aggregationKey. If not, we might have to use the key!
There was a problem hiding this comment.
we need to use the key!
There was a problem hiding this comment.
we should be able to build it on UI then ig.
src/routes/(console)/organization-[organization]/billing/planSummary.svelte
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/routes/(console)/organization-[organization]/billing/planSummary.svelte(6 hunks)
🔇 Additional comments (1)
src/routes/(console)/organization-[organization]/billing/planSummary.svelte (1)
26-27: Fix page store import and subscription
$app/statedoesn’t exportpage, and readingpage.urlhits the store object instead of its value—sopage.urlisundefined, breaking pagination as soon as the component mounts. Import the store from$app/storesand access$pageinside the derived runes to keep the table responsive to URL changes.-import { page } from '$app/state'; +import { page } from '$app/stores'; … -const projectsLimit = $derived(limit ?? (Number(page.url.searchParams.get('limit')) || 5)); +const projectsLimit = $derived(limit ?? (Number($page.url.searchParams.get('limit')) || 5)); const projectsOffset = $derived( - offset ?? ((Number(page.url.searchParams.get('page')) || 1) - 1) * projectsLimit + offset ?? ((Number($page.url.searchParams.get('page')) || 1) - 1) * projectsLimit );Also applies to: 162-165
src/routes/(console)/organization-[organization]/billing/planSummary.svelte
Show resolved
Hide resolved
| limit?: number; | ||
| offset?: number; |
There was a problem hiding this comment.
does the endpoint now allow these? 👀
quite nice if it fully supports pagination, does it also support Query.select?
speaking of queries, lets make this as a queries param instead of passing every param individually. Refer - https://github.com/appwrite/console/blob/main/src/lib/sdk/billing.ts#L455
| limit: getLimit(url, route, 5), | ||
| offset: pageToOffset(getPage(url) || 1, getLimit(url, route, 5)), | ||
| // unique key to force component refresh on page change | ||
| aggregationKey: `agg:${getPage(url) || 1}:${getLimit(url, route, 5)}` |
There was a problem hiding this comment.
this still applies @HarshMN2345?
There was a problem hiding this comment.
we should be able to build it on UI then ig.
src/routes/(console)/organization-[organization]/billing/planSummary.svelte
Outdated
Show resolved
Hide resolved
HarshMN2345
changed the title
HarshMN2345
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/routes/(console)/organization-[organization]/billing/planSummary.svelte (1)
347-351: Prevent NaN when availableCredit is undefined.
availableCreditis typed asnumber | undefined. When undefined,Math.min(..., availableCredit)returnsNaN, which propagates intototalAmountand rendersNaNin the UI.Apply this diff to default
availableCreditto zero:const creditsApplied = $derived( - Math.min(currentAggregation?.amount ?? currentPlan?.price ?? 0, availableCredit) + Math.min(currentAggregation?.amount ?? currentPlan?.price ?? 0, availableCredit ?? 0) ); -const totalAmount = $derived(Math.max(currentAggregation?.amount - creditsApplied, 0)); +const totalAmount = $derived( + Math.max((currentAggregation?.amount ?? currentPlan?.price ?? 0) - creditsApplied, 0) +);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/routes/(console)/organization-[organization]/billing/planSummary.svelte(6 hunks)src/routes/(console)/organization-[organization]/billing/planSummaryOld.svelte(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-05T09:41:40.439Z
Learnt from: ItzNotABug
PR: appwrite/console#2398
File: src/routes/(console)/verify-email/+page.svelte:48-51
Timestamp: 2025-10-05T09:41:40.439Z
Learning: In SvelteKit 5, `page` imported from `$app/state` is a reactive state object (using runes), not a store. It should be accessed as `page.data` without the `$` prefix, unlike the store-based `$page` from `$app/stores` in earlier versions.
Applied to files:
src/routes/(console)/organization-[organization]/billing/planSummary.svelte
🔇 Additional comments (5)
src/routes/(console)/organization-[organization]/billing/planSummaryOld.svelte (1)
202-202: LGTM! Type-safe event tracking.The change from a string literal to the
Click.OrganizationClickUpgradeenum constant improves type safety and aligns with the event tracking pattern used elsewhere in the codebase.src/routes/(console)/organization-[organization]/billing/planSummary.svelte (4)
26-26: Import location and reactivity: verify SvelteKit 5 usage.The
pageimport from$app/stateis correct for SvelteKit 5 with runes. However, based on the retrieved learning,pagein SvelteKit 5 is a reactive state object that should be accessed directly (without$). Your usage on lines 162-165 (page.url.searchParams.get(...)) appears correct for runes.Based on learnings
28-44: LGTM! Svelte 5 props syntax.The migration from
export letto the runes-basedlet {...} = $props()syntax is correct for Svelte 5. The new props (limit,offset,aggregationKey) support the pagination feature.
162-171: Verify reactivity of derived pagination values.The derived pagination calculations depend on
page.url.searchParamsand props. Ensure that changes to URL params trigger re-computation as expected in your Svelte 5 setup.You can verify this by:
- Testing navigation with different
?page=N&limit=Mquery params- Confirming that
projectsLimit,projectsOffset, andtotalProjectsupdate correctly
355-618: LGTM! AccordionTable migration with pagination.The refactor from
ExpandableTabletoAccordionTableis well-structured. The{#key aggregationKey}block ensures component remounting when aggregation data changes, and the pagination integration is clean.
| } | ||
| } | ||
| /* reducingh size of paginator */ |
There was a problem hiding this comment.
Fix typo in CSS comment.
"reducingh" should be "reducing".
Apply this diff:
- /* reducingh size of paginator */
+ /* reducing size of paginator */📝 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.
| /* reducingh size of paginator */ | |
| /* reducing size of paginator */ |
🤖 Prompt for AI Agents
In src/routes/(console)/organization-[organization]/billing/planSummary.svelte
at line 767, the CSS comment contains a typo "reducingh"; update the comment to
read "reducing" (e.g., change "reducingh size of paginator" to "reducing size of
paginator") so the comment is corrected but no functional code changes are made.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/routes/(console)/organization-[organization]/billing/+page.ts (1)
41-49: Sanitize page/limit before computing offset and calling the SDK.
getPageandgetLimitcan returnNaNor non-positive values from malformed URL parameters. Line 41's|| 1fallback handles falsy values but notNaNor negatives. This results in invalid offset calculations and SDK arguments.This concern was previously flagged. Apply the suggested validation:
- const currentPage = getPage(url) || 1; - const limit = getLimit(url, route, 5); - const offset = pageToOffset(currentPage, limit); + const rawPage = getPage(url); + const rawLimit = getLimit(url, route, 5); + const currentPage = + Number.isFinite(rawPage) && rawPage > 0 ? Math.floor(rawPage) : 1; + const limit = + Number.isFinite(rawLimit) && rawLimit > 0 ? Math.floor(rawLimit) : 5; + const offset = pageToOffset(currentPage, limit);src/routes/(console)/organization-[organization]/billing/planSummary.svelte (2)
349-353: Guard against NaN when credits or amount are missing.When
availableCreditisundefined,Math.minreturnsNaN, which propagates intototalAmountand renders asNaNin the UI. Additionally, line 353 lacks a fallback forcurrentAggregation?.amount.Apply this diff to prevent NaN:
-const creditsApplied = $derived( - Math.min(currentAggregation?.amount ?? currentPlan?.price ?? 0, availableCredit) -); - -const totalAmount = $derived(Math.max(currentAggregation?.amount - creditsApplied, 0)); +const creditsApplied = $derived( + Math.min( + currentAggregation?.amount ?? currentPlan?.price ?? 0, + availableCredit ?? 0 + ) +); + +const totalAmount = $derived( + Math.max( + (currentAggregation?.amount ?? currentPlan?.price ?? 0) - creditsApplied, + 0 + ) +);
769-769: Fix typo in CSS comment."reducingh" should be "reducing".
Apply this diff:
- /* reducingh size of paginator */ + /* reducing size of paginator */
🧹 Nitpick comments (1)
src/routes/(console)/organization-[organization]/billing/planSummary.svelte (1)
164-169: Simplify totalProjects calculation.The
?? nullfollowed by||is redundant. Consider using the nullish coalescing operator consistently.Apply this diff:
-const totalProjects = $derived( - (currentAggregation?.resources?.find?.((r) => r.resourceId === 'projects')?.value ?? - null) || - currentAggregation?.breakdown?.length || - 0 -); +const totalProjects = $derived( + currentAggregation?.resources?.find?.((r) => r.resourceId === 'projects')?.value ?? + currentAggregation?.breakdown?.length ?? + 0 +);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/routes/(console)/organization-[organization]/billing/+page.ts(4 hunks)src/routes/(console)/organization-[organization]/billing/planSummary.svelte(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-05T09:41:40.439Z
Learnt from: ItzNotABug
PR: appwrite/console#2398
File: src/routes/(console)/verify-email/+page.svelte:48-51
Timestamp: 2025-10-05T09:41:40.439Z
Learning: In SvelteKit 5, `page` imported from `$app/state` is a reactive state object (using runes), not a store. It should be accessed as `page.data` without the `$` prefix, unlike the store-based `$page` from `$app/stores` in earlier versions.
Applied to files:
src/routes/(console)/organization-[organization]/billing/planSummary.svelte
🧬 Code graph analysis (1)
src/routes/(console)/organization-[organization]/billing/+page.ts (3)
src/routes/(console)/organization-[organization]/+page.ts (1)
load(9-43)src/lib/helpers/load.ts (3)
getPage(8-10)getLimit(12-16)pageToOffset(4-6)src/lib/stores/organization.ts (2)
organization(62-62)Organization(16-38)
🔇 Additional comments (1)
src/routes/(console)/organization-[organization]/billing/planSummary.svelte (1)
160-163: The usage ofpage.urlis correct for SvelteKit 5.The past review comment suggesting
$page.urlis incorrect. Sincepageis imported from$app/state(line 26), it's a reactive state object using SvelteKit 5 runes, not a store. Accessing it aspage.urlwithout the$prefix is the correct approach.Based on learnings.
| limit: getLimit(url, route, 5), | ||
| offset: pageToOffset(getPage(url) || 1, getLimit(url, route, 5)) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid recalculating limit and offset.
limit and offset are already computed at lines 42-43. Recalculating them here duplicates logic and risks inconsistency.
Apply this diff to reuse the existing values:
- limit: getLimit(url, route, 5),
- offset: pageToOffset(getPage(url) || 1, getLimit(url, route, 5))
+ limit,
+ offset📝 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.
| limit: getLimit(url, route, 5), | |
| offset: pageToOffset(getPage(url) || 1, getLimit(url, route, 5)) | |
| - limit: getLimit(url, route, 5), | |
| limit, | |
| offset |
🤖 Prompt for AI Agents
In src/routes/(console)/organization-[organization]/billing/+page.ts around
lines 98 to 99 (previously computed at lines 42-43), the code recalculates limit
and offset via getLimit/getPage/pageToOffset; instead reuse the existing limit
and offset variables computed earlier instead of calling getLimit/getPage
again—replace the duplicate calls so the object uses the previously computed
limit and the previously computed offset (or compute offset once using the
existing page and limit variables) to avoid logic duplication and inconsistency.
2 participants
What does this PR do?
Show badge with count next to “Additional projects”
Implement backend-driven pagination (limit/offset) for project breakdown
Update SDK getAggregation(orgId, aggregationId, limit?, offset?)
Wire loader to read page/limit from URL and pass to component
Add dependency key and {#key} to force reload on page changes
Test Plan
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?
yes
Summary by CodeRabbit
New Features
UI
Chores