Feat: add screenshotsGenerated metric across console #2542
Conversation
…nts)/logsTimer.svelte Co-authored-by: Copilot <[email protected]>
ConsoleProject ID: Tip You can use Avatars API to generate QR code for any text or URLs. |
WalkthroughThis pull request introduces two major feature areas: (1) a new billing metric category for screenshots generated, extending usage tracking and billing plan structures across multiple data models, UI components, and pages; and (2) a build timeout detection system that calculates whether deployments have exceeded configured timeout thresholds, with effective status computation applied across deployment and site-related components to reflect timeout-induced failures. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–75 minutes Key areas requiring extra attention:
Pre-merge checks and finishing touches✅ Passed checks (2 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: 3
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)/project-[region]-[project]/sites/(components)/deploymentActionMenu.svelte (1)
134-145: Inconsistent status check for delete button.Line 134 checks
deployment.statusdirectly instead of usingeffectiveStatuslike all other status-based conditionals in this file (lines 80, 92, 111, 122). This inconsistency could lead to incorrect behavior when a deployment has timed out.Apply this diff to use effectiveStatus consistently:
- {#if ['ready', 'failed'].includes(deployment.status)} + {#if ['ready', 'failed'].includes(effectiveStatus)} <ActionMenu.Item.Button status="danger" leadingIcon={IconTrash}src/routes/(console)/project-[region]-[project]/functions/function-[function]/table.svelte (1)
160-170: Inconsistent status check for activate button.Line 160 checks
deployment.status === 'ready'instead of usingeffectiveStatuslike the rest of the file (lines 109-110, 117, 119, 174, 187). This could prevent activation of deployments that have actually completed but whose status hasn't been updated.Apply this diff to use effectiveStatus consistently:
- {#if deployment.status === 'ready' && deployment.$id !== $func.deploymentId} + {#if effectiveStatus === 'ready' && deployment.$id !== $func.deploymentId} <ActionMenu.Item.Button leadingIcon={IconLightningBolt}src/routes/(console)/project-[region]-[project]/functions/function-[function]/deployment-[deployment]/+page.svelte (1)
127-138: Delete gating ignores timeout statusThe delete action still checks
data.deployment.status. When a deployment times out,effectiveStatusbecomes'failed', but the original status remains'building', so the delete option never appears. Switch this condition to referenceeffectiveStatusto keep the UI consistent with the new timeout logic.- {#if $canWriteFunctions && ['ready', 'failed'].includes(data.deployment.status)} + {#if $canWriteFunctions && ['ready', 'failed'].includes(effectiveStatus)}
🧹 Nitpick comments (1)
src/routes/(console)/project-[region]-[project]/functions/function-[function]/(components)/deploymentCard.svelte (1)
134-134: Consider capitalizing the label for consistency.The
Statuscomponent now correctly useseffectiveStatus, but unlikesiteCard.svelte(line 129), the label isn't capitalized. Consider usinglabel={capitalize(effectiveStatus)}for consistency across components.Apply this diff for consistency:
- <Status status={effectiveStatus} label={effectiveStatus} /> + <Status status={effectiveStatus} label={capitalize(effectiveStatus)} />
📜 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 (20)
package.json(1 hunks)src/lib/helpers/buildTimeout.ts(1 hunks)src/lib/sdk/billing.ts(4 hunks)src/lib/sdk/usage.ts(1 hunks)src/lib/stores/billing.ts(1 hunks)src/routes/(console)/organization-[organization]/billing/planSummary.svelte(2 hunks)src/routes/(console)/organization-[organization]/usage/[[invoice]]/+page.svelte(1 hunks)src/routes/(console)/organization-[organization]/usage/[[invoice]]/+page.ts(1 hunks)src/routes/(console)/organization-[organization]/usage/[[invoice]]/ProjectBreakdown.svelte(2 hunks)src/routes/(console)/project-[region]-[project]/functions/function-[function]/(components)/deploymentCard.svelte(3 hunks)src/routes/(console)/project-[region]-[project]/functions/function-[function]/deployment-[deployment]/+page.svelte(5 hunks)src/routes/(console)/project-[region]-[project]/functions/function-[function]/table.svelte(5 hunks)src/routes/(console)/project-[region]-[project]/settings/usage/[[invoice]]/+page.svelte(2 hunks)src/routes/(console)/project-[region]-[project]/sites/(components)/deploymentActionMenu.svelte(6 hunks)src/routes/(console)/project-[region]-[project]/sites/(components)/logs.svelte(3 hunks)src/routes/(console)/project-[region]-[project]/sites/(components)/logsTimer.svelte(1 hunks)src/routes/(console)/project-[region]-[project]/sites/(components)/siteCard.svelte(3 hunks)src/routes/(console)/project-[region]-[project]/sites/site-[site]/deployments/deployment-[deployment]/+page.svelte(4 hunks)src/routes/(console)/project-[region]-[project]/sites/site-[site]/deployments/table.svelte(3 hunks)src/routes/(console)/project-[region]-[project]/sites/site-[site]/deploymentsOverview.svelte(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-26T06:48:57.938Z
Learnt from: ItzNotABug
Repo: appwrite/console PR: 2373
File: src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte:629-631
Timestamp: 2025-09-26T06:48:57.938Z
Learning: In the Appwrite console codebase using appwrite.io/pink-svelte, the Icon component automatically handles CSS variable names passed to its color prop by internally wrapping them with var(). Therefore, passing '--some-css-variable' as a string to the Icon color prop works correctly without needing to manually wrap it with var().
Applied to files:
src/routes/(console)/project-[region]-[project]/functions/function-[function]/deployment-[deployment]/+page.sveltepackage.json
📚 Learning: 2025-10-13T05:16:07.656Z
Learnt from: ItzNotABug
Repo: appwrite/console PR: 2413
File: src/routes/(console)/project-[region]-[project]/databases/database-[database]/header.svelte:54-58
Timestamp: 2025-10-13T05:16:07.656Z
Learning: In SvelteKit apps, shared layout components (like headers) that use `$derived(page.data.*)` should use optional chaining when accessing properties that may not be present on all routes. During page transitions, reactive statements can briefly evaluate with different page.data structures, so optional chaining prevents runtime errors when navigating between routes with different data shapes (e.g., between `/databases` and `/databases/database-[database]`).
Applied to files:
src/routes/(console)/project-[region]-[project]/functions/function-[function]/deployment-[deployment]/+page.svelte
📚 Learning: 2025-10-05T09:41:40.439Z
Learnt from: ItzNotABug
Repo: appwrite/console PR: 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)/project-[region]-[project]/functions/function-[function]/deployment-[deployment]/+page.svelte
🧬 Code graph analysis (2)
src/lib/helpers/buildTimeout.ts (1)
src/routes/(console)/store.ts (1)
consoleVariables(9-12)
src/lib/sdk/billing.ts (1)
src/lib/sdk/usage.ts (1)
Metric(21-30)
🪛 ESLint
src/routes/(console)/project-[region]-[project]/settings/usage/[[invoice]]/+page.svelte
[error] 33-33: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 34-34: 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 (17)
package.json (1)
25-25: LGTM!The dependency update to
@appwrite.io/consolebrings in support for the new screenshotsGenerated metric and build timeout features introduced in this PR.src/routes/(console)/organization-[organization]/usage/[[invoice]]/+page.ts (1)
37-39: LGTM!The addition of
screenshotsGeneratedandscreenshotsGeneratedTotalfields follows the established pattern for the migration fallback case. The fields are correctly set to null and placed logically after the relatedimageTransformationsfields.src/routes/(console)/organization-[organization]/usage/[[invoice]]/ProjectBreakdown.svelte (2)
19-20: LGTM!The addition of
'screenshotsGenerated'to theMetrictype union is correct and follows the established pattern. The placement after'imageTransformations'maintains logical grouping of related metrics.
92-95: LGTM!The formatting logic for
screenshotsGeneratedcorrectly reuses theformatNumberWithCommasformatter, which is appropriate for count-based metrics and consistent with the handling ofimageTransformations.src/routes/(console)/project-[region]-[project]/sites/(components)/siteCard.svelte (4)
26-27: LGTM!The imports for
getEffectiveBuildStatusandregionalConsoleVariablesare correctly placed and necessary for the build timeout detection feature.
45-47: LGTM!The
effectiveStatusderived value correctly computes the effective build status using the deployment's status, creation timestamp, and regional console variables. This enables timeout-based status detection.
121-121: LGTM!The condition now correctly uses
effectiveStatusinstead of the rawdeployment.status, which ensures that timeout-induced failures are properly displayed in the UI.
128-129: LGTM!The
Statuscomponent now correctly displays theeffectiveStatusinstead of the raw deployment status, ensuring that timeout-induced failures are properly reflected in the UI.src/routes/(console)/project-[region]-[project]/functions/function-[function]/(components)/deploymentCard.svelte (3)
18-19: LGTM!The imports for
getEffectiveBuildStatusandregionalConsoleVariablesare correctly placed and follow the same pattern as other deployment-related components.
41-43: LGTM!The
effectiveStatusderived value is correctly implemented using the same pattern as other deployment components, ensuring consistent timeout detection across the console.
130-130: LGTM!The condition correctly uses
effectiveStatusto detect both explicit failures and timeout-induced failures in function deployments.src/lib/sdk/usage.ts (1)
336-344: LGTM!The addition of
screenshotsGeneratedandscreenshotsGeneratedTotalfields to theUsageProjecttype is well-documented and follows the established pattern used forimageTransformations. The JSDoc comments clearly describe the purpose of each field.src/routes/(console)/organization-[organization]/billing/planSummary.svelte (2)
130-132: LGTM!The extraction of
screenshotsGeneratedresource data follows the established pattern and is correctly placed in the project data mapping.
286-298: LGTM!The Screenshots generated billing line item is correctly implemented following the same pattern as other metrics. The usage display, progress bar, and price formatting are all properly configured.
src/lib/stores/billing.ts (1)
153-154: LGTM!The addition of
'screenshotsGenerated'to thePlanServicesunion type is correct and enables the billing logic to handle screenshot generation limits through thegetServiceLimitfunction.src/routes/(console)/project-[region]-[project]/settings/usage/[[invoice]]/+page.svelte (1)
246-283: LGTM! Consistent implementation of the new metric.The screenshots generated card follows the same pattern as other metrics (e.g., image transformations at lines 208-245), with proper formatting, chart configuration, and empty state handling.
src/routes/(console)/organization-[organization]/usage/[[invoice]]/+page.svelte (1)
325-370: LGTM! Properly integrated screenshots metric.The implementation mirrors the structure of other metrics (bandwidth, users, executions) with correct chart configuration, progress bar display, and project breakdown integration.
| let effectiveStatus = $derived( | ||
| getEffectiveBuildStatus( | ||
| data.deployment.status, | ||
| data.deployment.$createdAt, | ||
| $regionalConsoleVariables | ||
| ) | ||
| ); |
There was a problem hiding this comment.
Timeout logic never re-runs
Same as in logsTimer.svelte: the derived effectiveStatus has no time-based dependency, so once the component mounts it never re-checks the timeout and deployments stuck in building/processing never graduate to 'failed'. Please introduce a ticking dependency (e.g. a $state updated on an interval or a SvelteDate) so the timeout calculation re-executes after the configured threshold.
| $: screenshotsGenerated = (data.usage as any).screenshotsGenerated ?? null; | ||
| $: screenshotsGeneratedTotal = (data.usage as any).screenshotsGeneratedTotal ?? 0; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace explicit any type cast with proper typing.
The explicit any type cast bypasses TypeScript's type checking. The data.usage type definition should be updated to include the new screenshotsGenerated and screenshotsGeneratedTotal fields rather than using a type assertion.
Consider updating the type definition for data.usage to include:
{
// ... existing fields
screenshotsGenerated?: Array<Models.Metric>;
screenshotsGeneratedTotal?: number;
}Then remove the any cast:
- $: screenshotsGenerated = (data.usage as any).screenshotsGenerated ?? null;
- $: screenshotsGeneratedTotal = (data.usage as any).screenshotsGeneratedTotal ?? 0;
+ $: screenshotsGenerated = data.usage.screenshotsGenerated ?? null;
+ $: screenshotsGeneratedTotal = data.usage.screenshotsGeneratedTotal ?? 0;🧰 Tools
🪛 ESLint
[error] 33-33: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 34-34: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
🤖 Prompt for AI Agents
In
src/routes/(console)/project-[region]-[project]/settings/usage/[[invoice]]/+page.svelte
around lines 33-34, remove the explicit any cast on data.usage and instead
update the TypeScript type for data.usage to include screenshotsGenerated?:
Models.Metric[] and screenshotsGeneratedTotal?: number; then update any shared
usage/interface type (or the load function return type) so Svelte knows these
fields exist and you can directly reference data.usage.screenshotsGenerated and
data.usage.screenshotsGeneratedTotal without using "as any".
| let effectiveStatus = $derived( | ||
| getEffectiveBuildStatus(status, deployment.$createdAt, $regionalConsoleVariables) | ||
| ); |
There was a problem hiding this comment.
Timeout logic never re-runs
$derived only re-evaluates when its dependencies change. Here, those dependencies (status, deployment.$createdAt, $regionalConsoleVariables) stay constant while a build is stuck. As a result, the timeout check (which reads Date.now()) fires once and effectiveStatus never flips to 'failed', so timed-out builds remain shown as “building” forever. Please add a time-based dependency (for example, drive a $state updated via setInterval, or use a reactive SvelteDate) so the derived value recomputes as time passes.
|
closed in favour of #2543 |
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
Improvements