feat: pagination on billing ui table #2350
Conversation
ConsoleProject ID: Sites (2)
Note Cursor pagination performs better than offset pagination when loading further pages. |
Walkthrough
Possibly related PRs
Suggested reviewers
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing Touches
🧪 Generate unit tests
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.
| const currentPage = getPage(url) || 1; | ||
| const limit = getLimit(url, route, 5); | ||
| const offset = pageToOffset(currentPage, limit); | ||
| billingAggregation = await sdk.forConsole.billing.getAggregation( | ||
| organization.$id, | ||
| (organization as Organization)?.billingAggregationId | ||
| (organization as Organization)?.billingAggregationId, | ||
| limit, | ||
| offset | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Sanitize page/limit to avoid NaN/invalid values reaching the SDK.
getPage/getLimit can return NaN or <=0 from URL. Guard and normalize before computing offset and calling the SDK.
Apply:
- 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);📝 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.
| const currentPage = getPage(url) || 1; | |
| const limit = getLimit(url, route, 5); | |
| const offset = pageToOffset(currentPage, limit); | |
| billingAggregation = await sdk.forConsole.billing.getAggregation( | |
| organization.$id, | |
| (organization as Organization)?.billingAggregationId | |
| (organization as Organization)?.billingAggregationId, | |
| limit, | |
| offset | |
| ); | |
| 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); | |
| billingAggregation = await sdk.forConsole.billing.getAggregation( | |
| organization.$id, | |
| (organization as Organization)?.billingAggregationId, | |
| limit, | |
| offset | |
| ); |
🤖 Prompt for AI Agents
In src/routes/(console)/organization-[organization]/billing/+page.ts around
lines 40 to 48, getPage/getLimit can return NaN or non-positive values from the
URL which then produce an invalid offset and bad args for the SDK; before
computing offset and calling sdk.forConsole.billing.getAggregation, coerce and
validate the values: parse/convert the returned page and limit to numbers, use
Number.isFinite and >0 checks, fallback page to 1 and limit to the existing
default (5) when invalid, then compute offset with the sanitized integers and
pass those to the SDK; ensure types remain number and avoid passing NaN or <=0
to pageToOffset or the SDK.
| (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.
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
Improvements
Chores