feat: add build timeout handling to prevent stuck build UI #2520
Conversation
|
This PR was not deployed automatically as @HarshMN2345 does not have access to the Railway project. In order to get automatic PR deploys, please add @HarshMN2345 to your workspace on Railway. |
ConsoleProject ID: Tip Cursor pagination performs better than offset pagination when loading further pages. |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a new helper at src/lib/helpers/buildTimeout.ts that exposes getEffectiveBuildStatus(originalStatus, createdAt, consoleVariables) to compute an effective deployment status when a build timeout has elapsed. The helper contains two internal functions — isBuildTimedOut(createdAt, status, timeoutSeconds) and getBuildTimeoutSeconds(consoleVariables) — which are not exported. Many Svelte components under functions and sites were updated to import getEffectiveBuildStatus and regionalConsoleVariables, compute effectiveStatus (using deployment.$createdAt) and replace direct uses of deployment.status with effectiveStatus for badges, timers, labels, action visibility, and logs. logsTimer.svelte and a few other components now read props via $props() instead of exported props. package.json updates the @appwrite.io/console dependency reference. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (3)📚 Learning: 2025-09-26T06:48:57.938ZApplied to files:
📚 Learning: 2025-10-13T05:16:07.656ZApplied to files:
📚 Learning: 2025-10-05T09:41:40.439ZApplied to files:
🧬 Code graph analysis (1)src/lib/helpers/buildTimeout.ts (1)
⏰ 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). (2)
🔇 Additional comments (9)
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 |
There was a problem hiding this comment.
Pull Request Overview
This PR adds build timeout handling to prevent deployments from appearing stuck in the UI. When a deployment exceeds the configured build timeout, it is automatically displayed as "failed" in the interface.
Key changes:
- Introduced a new
buildTimeout.tshelper module with timeout detection logic - Updated all deployment status displays across sites and functions to use the effective (timeout-aware) status
- Modified action menus to properly enable/disable actions based on the effective status
Reviewed Changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/lib/helpers/buildTimeout.ts |
New helper module containing timeout detection and status computation logic |
src/routes/(console)/project-[region]-[project]/sites/site-[site]/deploymentsOverview.svelte |
Updated to display effective status considering timeouts |
src/routes/(console)/project-[region]-[project]/sites/site-[site]/deployments/table.svelte |
Updated status display and build duration logic to use effective status |
src/routes/(console)/project-[region]-[project]/sites/site-[site]/deployments/deployment-[deployment]/+page.svelte |
Updated deployment detail page to show effective status |
src/routes/(console)/project-[region]-[project]/sites/(components)/siteCard.svelte |
Updated site card status display |
src/routes/(console)/project-[region]-[project]/sites/(components)/logsTimer.svelte |
Updated timer display logic to use effective status |
src/routes/(console)/project-[region]-[project]/sites/(components)/logs.svelte |
Updated log display conditions to use effective status |
src/routes/(console)/project-[region]-[project]/sites/(components)/deploymentActionMenu.svelte |
Updated action menu availability based on effective status |
src/routes/(console)/project-[region]-[project]/functions/function-[function]/table.svelte |
Updated function deployment table to use effective status |
src/routes/(console)/project-[region]-[project]/functions/function-[function]/deployment-[deployment]/+page.svelte |
Updated function deployment detail page |
src/routes/(console)/project-[region]-[project]/functions/function-[function]/(components)/deploymentCard.svelte |
Updated deployment card status display |
package.json |
Updated console package reference |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| {@const effectiveStatus = getEffectiveBuildStatus( | ||
| deployment.status, | ||
| deployment.$createdAt, | ||
| getBuildTimeoutSeconds($regionalConsoleVariables) | ||
| )} |
There was a problem hiding this comment.
The effectiveStatus is computed twice in the same template block (lines 99-103 and 116-120). This duplicates logic and could lead to inconsistencies. Consider computing it once at the top of the iteration block and reusing the value.
| {@const effectiveStatus = getEffectiveBuildStatus( | ||
| deployment.status, | ||
| deployment.$createdAt, | ||
| getBuildTimeoutSeconds($regionalConsoleVariables) | ||
| )} |
There was a problem hiding this comment.
The effectiveStatus is computed twice in the same template block (lines 119-123 and 137-141). This duplicates logic and could lead to inconsistencies. Consider computing it once at the top of the iteration block and reusing the value.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (4)
src/routes/(console)/project-[region]-[project]/functions/function-[function]/deployment-[deployment]/+page.svelte (1)
155-161: Unify logs header with effectiveStatus to avoid conflicting signals.Logs badge and spinner still read raw status. Use effectiveStatus so the header reflects timeout-induced failure immediately.
@@ - <Accordion - title="Deployment logs" - badge={capitalize(data.deployment.status)} - open - badgeType={badgeTypeDeployment(data.deployment.status)} - hideDivider> + <Accordion + title="Deployment logs" + badge={capitalize(effectiveStatus)} + open + badgeType={badgeTypeDeployment(effectiveStatus)} + hideDivider> @@ - {#if ['processing', 'building'].includes(data.deployment.status)} + {#if ['processing', 'building'].includes(effectiveStatus)} <Typography.Code color="--fgcolor-neutral-secondary"> <Layout.Stack direction="row" alignItems="center" inline> <p use:timer={{ start: data.deployment.$createdAt }}></p> <Spinner size="s" /> </Layout.Stack> </Typography.Code> {:else}Optional: also invalidate on a terminal failure to refresh duration/logs from the API.
- if (message.payload.status === 'ready') { + if (['ready', 'failed'].includes(message.payload.status)) { invalidate(Dependencies.DEPLOYMENT); }Also applies to: 171-184
src/routes/(console)/project-[region]-[project]/sites/(components)/deploymentActionMenu.svelte (1)
75-115: Critical inconsistency: mixed usage of raw status and effectiveStatus.Lines 75, 87, and 106 use
deployment?.statusdirectly, while lines 122 and 134 useeffectiveStatus. This creates inconsistent behavior:
- Activate button (line 75): Uses raw
status === 'ready'- Download submenu (lines 87, 106): Uses raw
status- Cancel button (line 122): Uses
effectiveStatus- Delete button (line 134): Uses
effectiveStatusIf a deployment times out and its
effectiveStatusbecomes'failed'whilestatusremains'building':
- Cancel button would incorrectly disappear (checks effectiveStatus)
- Download submenu would incorrectly remain visible (checks raw status)
- The UI would show conflicting signals to the user
Apply this diff to use effectiveStatus consistently:
+ {@const effectiveStatus = getEffectiveBuildStatus( + deployment.status, + deployment.$createdAt, + getBuildTimeoutSeconds($regionalConsoleVariables) + )} {#if !inCard} <Tooltip disabled={selectedDeployment?.sourceSize !== 0} placement={'bottom'}> <div> @@ -72,7 +77,7 @@ <div slot="tooltip">Source is empty</div> </Tooltip> {/if} - {#if deployment?.status === 'ready' && deployment?.$id !== activeDeployment} + {#if effectiveStatus === 'ready' && deployment?.$id !== activeDeployment} <ActionMenu.Item.Button leadingIcon={IconLightningBolt} on:click={(e) => { @@ -84,7 +89,7 @@ Activate </ActionMenu.Item.Button> {/if} - {#if deployment?.status === 'ready' || deployment?.status === 'failed' || deployment?.status === 'building'} + {#if effectiveStatus === 'ready' || effectiveStatus === 'failed' || effectiveStatus === 'building'} <SubMenu> <ActionMenu.Root noPadding> <ActionMenu.Item.Button @@ -103,7 +108,7 @@ </ActionMenu.Item.Anchor> <ActionMenu.Item.Anchor - disabled={deployment?.status !== 'ready'} + disabled={effectiveStatus !== 'ready'} on:click={toggle} href={getOutputDownload(deployment.$id)} external> @@ -114,8 +119,6 @@ </SubMenu> {/if} - {@const effectiveStatus = getEffectiveBuildStatus( - deployment.status, - deployment.$createdAt, - getBuildTimeoutSeconds($regionalConsoleVariables) - )}Note: Move the
effectiveStatuscomputation to the top of the menu block so it can be used by all conditions.src/routes/(console)/project-[region]-[project]/sites/site-[site]/deployments/deployment-[deployment]/+page.svelte (1)
65-70: Inconsistent status check for Visit button visibility.Line 65 checks
deployment?.status === 'ready'directly instead of usingeffectiveStatus. For consistency with the timeout handling feature, this should useeffectiveStatus.Apply this diff:
- {#if deployment?.status === 'ready' && data.proxyRuleList?.total} + {#if effectiveStatus === 'ready' && data.proxyRuleList?.total} <Button external href={`${$regionalProtocol}${data.proxyRuleList.rules[0]?.domain}`}>src/routes/(console)/project-[region]-[project]/sites/site-[site]/deployments/table.svelte (1)
86-150: Performance issue: effectiveStatus computed twice per row.The
effectiveStatusis computed separately for the status column (lines 99-103) and the buildDuration column (lines 116-120), resulting in duplicate function calls for each deployment row. For large deployment lists, this is inefficient.Refactor to compute
effectiveStatusonce per row:{#each data.deploymentList.deployments as deployment (deployment.$id)} + {@const effectiveStatus = getEffectiveBuildStatus( + deployment.status, + deployment.$createdAt, + getBuildTimeoutSeconds($regionalConsoleVariables) + )} <Table.Row.Link {root} id={deployment.$id} href={`${base}/project-${page.params.region}-${page.params.project}/sites/site-${page.params.site}/deployments/deployment-${deployment.$id}`}> {#each $columns as column} <Table.Cell column={column.id} {root}> {#if column.id === '$id'} {#key column.id} <Id value={deployment.$id}>{deployment.$id}</Id> {/key} {:else if column.id === 'status'} {@const status = deployment.status} - {@const effectiveStatus = getEffectiveBuildStatus( - status, - deployment.$createdAt, - getBuildTimeoutSeconds($regionalConsoleVariables) - )} {#if data?.activeDeployment?.$id === deployment?.$id} <Status status="complete" label="Active" /> {:else} @@ -114,11 +114,6 @@ {:else if column.id === '$updatedAt'} <DeploymentCreatedBy {deployment} /> {:else if column.id === 'buildDuration'} - {@const effectiveStatus = getEffectiveBuildStatus( - deployment.status, - deployment.$createdAt, - getBuildTimeoutSeconds($regionalConsoleVariables) - )} {#if ['waiting'].includes(effectiveStatus)} - {:else if ['processing', 'building'].includes(effectiveStatus)}
🧹 Nitpick comments (6)
src/routes/(console)/project-[region]-[project]/sites/(components)/siteCard.svelte (1)
45-51: Show “Status” for any non-ready effective status (not just failed).Today only failed renders a Status chip; processing/building/waiting end up under “Deployed,” which is confusing. Use the derived status for all non-ready states.
- let effectiveStatus = $derived( + let effectiveStatus = $derived( getEffectiveBuildStatus( deployment.status, deployment.$createdAt, getBuildTimeoutSeconds($regionalConsoleVariables) ) ); @@ - {#if effectiveStatus === 'failed'} + {#if effectiveStatus !== 'ready'} <Layout.Stack gap="xxs" inline> <Typography.Text variant="m-400" color="--fgcolor-neutral-tertiary"> - Status + Status </Typography.Text> <Typography.Text variant="m-400" color="--fgcolor-neutral-primary"> <Status - status={effectiveStatus} - label={capitalize(effectiveStatus)} /> + status={effectiveStatus} + label={capitalize(effectiveStatus)} /> </Typography.Text> </Layout.Stack> {:else}Also applies to: 125-145
src/routes/(console)/project-[region]-[project]/functions/function-[function]/table.svelte (1)
119-124: Compute effectiveStatus once per row and reuse.It’s recomputed in three places. Derive once per row to avoid drift and keep conditions in sync.
@@ - {#each columns as column} + {@const rowEffectiveStatus = getEffectiveBuildStatus( + deployment.status, + deployment.$createdAt, + getBuildTimeoutSeconds($regionalConsoleVariables) + )} + {#each columns as column} @@ - {@const effectiveStatus = getEffectiveBuildStatus( - status, - deployment.$createdAt, - getBuildTimeoutSeconds($regionalConsoleVariables) - )} + {@const effectiveStatus = rowEffectiveStatus} @@ - {@const effectiveStatus = getEffectiveBuildStatus( - deployment.status, - deployment.$createdAt, - getBuildTimeoutSeconds($regionalConsoleVariables) - )} + {@const effectiveStatus = rowEffectiveStatus} @@ - {@const effectiveStatus = getEffectiveBuildStatus( - deployment.status, - deployment.$createdAt, - getBuildTimeoutSeconds($regionalConsoleVariables) - )} + {@const effectiveStatus = rowEffectiveStatus}Additionally, consider centralizing the in‑progress list to avoid typos:
+<!-- near imports --> +<!-- const IN_PROGRESS_STATUSES = new Set(['waiting','processing','building']); --> @@ - {#if effectiveStatus === 'processing' || effectiveStatus === 'building' || effectiveStatus === 'waiting'} + {#if IN_PROGRESS_STATUSES.has(effectiveStatus)} @@ - {#if effectiveStatus !== 'building' && effectiveStatus !== 'processing' && effectiveStatus !== 'waiting'} + {#if !IN_PROGRESS_STATUSES.has(effectiveStatus)}Also applies to: 137-148, 197-229
src/lib/helpers/buildTimeout.ts (3)
11-24: Timeout edge and clock-skew considerations.
- Confirm “exceeded” should be strictly greater than timeoutSeconds; many UIs treat “at timeout” as timed out (>=).
- Client-clock skew can misclassify near the boundary; consider a small buffer (e.g., 5–10s) if acceptable.
- const elapsedSeconds = Math.floor((now.getTime() - created.getTime()) / 1000); - return elapsedSeconds > timeoutSeconds; + const elapsedSeconds = Math.floor((now.getTime() - created.getTime()) / 1000); + const SKEW_BUFFER_SEC = 5; // mitigate small clock/network skew + return elapsedSeconds >= timeoutSeconds + SKEW_BUFFER_SEC;
11-13: Avoid duplicating status lists; export a shared constant.In‑progress statuses are repeated across components. Centralize them here to keep UI in sync.
+export const IN_PROGRESS_STATUSES = new Set(['waiting', 'processing', 'building'] as const); @@ - if (!['waiting', 'processing', 'building'].includes(status)) { + if (!IN_PROGRESS_STATUSES.has(status as any)) { return false; }Then, import and reuse IN_PROGRESS_STATUSES in Svelte components to gate Cancel/Delete and timers.
Also applies to: 29-38
46-51: Stricter numeric parsing and types.parseInt will coerce strings like "600s" to 600. If you want stricter config, use Number(...) and validate finite positivity.
- const timeout = parseInt(String(consoleVariables._APP_COMPUTE_BUILD_TIMEOUT), 10); - return isNaN(timeout) ? 0 : timeout; + const raw = (consoleVariables._APP_COMPUTE_BUILD_TIMEOUT as unknown); + const timeout = typeof raw === 'number' ? raw : Number(raw); + return Number.isFinite(timeout) && timeout > 0 ? Math.trunc(timeout) : 0;src/routes/(console)/project-[region]-[project]/sites/(components)/logsTimer.svelte (1)
10-18: Optional: Simplify by accepting effectiveStatus directly.The parent component (
logs.svelteline 80) already computeseffectiveStatusand passes it as thestatusprop. This component then recomputeseffectiveStatusfrom that status, which is redundant.Consider either:
- Rename the prop to
effectiveStatusand use it directly without recomputation, OR- Keep current implementation but add a comment explaining that status may be either raw or effective status
Current implementation is functional but slightly inefficient. This is a minor optimization opportunity.
Also applies to: 22-22
📜 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 (12)
package.json(1 hunks)src/lib/helpers/buildTimeout.ts(1 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(3 hunks)src/routes/(console)/project-[region]-[project]/functions/function-[function]/table.svelte(4 hunks)src/routes/(console)/project-[region]-[project]/sites/(components)/deploymentActionMenu.svelte(3 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(3 hunks)src/routes/(console)/project-[region]-[project]/sites/site-[site]/deployments/table.svelte(2 hunks)src/routes/(console)/project-[region]-[project]/sites/site-[site]/deploymentsOverview.svelte(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/lib/helpers/buildTimeout.ts (1)
src/routes/(console)/store.ts (1)
consoleVariables(9-12)
🔇 Additional comments (9)
src/routes/(console)/project-[region]-[project]/functions/function-[function]/deployment-[deployment]/+page.svelte (1)
43-49: Good: centralized effectiveStatus + state runes.Derivation looks correct and Cancel gating uses effectiveStatus. No changes needed here.
Please confirm regionalConsoleVariables is populated before first render in this route; otherwise the derived value will recompute once variables arrive (expected).
Also applies to: 96-102
package.json (1)
25-25: Confirm build 2755 exports are compatible and pin is stable for production.The build pinning correlates with commit
5889e8c6d(add build timeout handling), which depends onModels.ConsoleVariablesand the_APP_COMPUTE_BUILD_TIMEOUTproperty. Verify that build 2755 exports these types correctly. Since pkg.pr.new artifacts are ephemeral, confirm this pin is:
- Intended for production or temporary
- Replaced with a stable release before merge
src/routes/(console)/project-[region]-[project]/functions/function-[function]/(components)/deploymentCard.svelte (2)
41-47: Good use of $derived for reactive effectiveStatus.The reactive derivation pattern is appropriate here. However, the same null safety concern applies: ensure
$regionalConsoleVariablesis always defined beforegetBuildTimeoutSecondsis called.
134-150: Verify the status display logic is intentional.The current implementation only displays the status row when
effectiveStatus === 'failed'. For all other states (including 'building', 'processing', 'ready'), it shows "Deployed" information instead.Is this intentional? Users might want to see when a deployment is still building or processing. Consider whether all deployment states should be visible in the deployment card.
src/routes/(console)/project-[region]-[project]/sites/(components)/deploymentActionMenu.svelte (1)
117-121: Consistent effectiveStatus computation.The pattern is consistent with other components. However, ensure null safety for
$regionalConsoleVariablesas mentioned in other files.src/routes/(console)/project-[region]-[project]/sites/site-[site]/deployments/deployment-[deployment]/+page.svelte (1)
29-35: Good reactive pattern for effectiveStatus.The derived reactive pattern is consistent with other components. Ensure null safety for
$regionalConsoleVariables.src/routes/(console)/project-[region]-[project]/sites/(components)/logs.svelte (1)
43-49: LGTM! Excellent implementation.This component demonstrates the correct pattern:
effectiveStatusis derived once (lines 43-49)- Used consistently throughout the component
- No mixed usage with raw
deployment.status- No duplicate computations
This is a good reference for how the other components should be refactored.
Also applies to: 52-64, 75-89
src/routes/(console)/project-[region]-[project]/sites/site-[site]/deploymentsOverview.svelte (2)
14-15: The buildTimeout helper implementation addresses all identified concerns.The exported functions handle edge cases properly:
getBuildTimeoutSecondsuses optional chaining and returns0whenconsoleVariablesis undefined- All functions include JSDoc documentation
isBuildTimedOutsafely validates status and timeout before calculating elapsed time, withisNaNhandling for invalid timeout valuesNo action required.
77-87: No null safety issue found—the original review comment is incorrect.
getBuildTimeoutSecondsexplicitly acceptsModels.ConsoleVariables | undefinedand safely handles undefined input with optional chaining (if (!consoleVariables?._APP_COMPUTE_BUILD_TIMEOUT)), returning 0 as a safe default. The code is already null-safe and requires no changes.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated 2 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/lib/helpers/buildTimeout.ts
Outdated
|
|
||
| const now = new Date(); | ||
| const created = new Date(createdAt); | ||
| const elapsedSeconds = Math.floor((now.getTime() - created.getTime()) / 1000); |
There was a problem hiding this comment.
The timeout check recalculates elapsed time on every render. Consider memoizing or caching the result when the status hasn't changed, especially for components that render frequently.
src/routes/(console)/project-[region]-[project]/sites/(components)/logsTimer.svelte
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/routes/(console)/project-[region]-[project]/sites/(components)/logsTimer.svelte
Outdated
Show resolved
Hide resolved
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 (1)
src/routes/(console)/project-[region]-[project]/functions/function-[function]/deployment-[deployment]/+page.svelte (1)
130-132: Duplicate condition; simplify and ensure non‑zero check.The OR repeats the same condition. Prefer an explicit > 0 check.
- {#if !!data.deployment?.sourceSize || !!data.deployment?.sourceSize} + {#if (data.deployment?.sourceSize ?? 0) > 0}
🧹 Nitpick comments (6)
src/routes/(console)/project-[region]-[project]/functions/function-[function]/deployment-[deployment]/+page.svelte (3)
173-184: Show spinner for ‘waiting’ as well for consistent “pending” UX.Currently only ‘processing/building’ show the live timer/spinner. ‘waiting’ appears static despite being pending.
- {#if ['processing', 'building'].includes(effectiveStatus)} + {#if ['processing', 'building', 'waiting'].includes(effectiveStatus)}
155-187: Timed‑out case may show ambiguous duration.When effectiveStatus becomes ‘failed’ due to timeout, buildDuration may be 0/undefined. Consider showing a clear “Timed out after X” using the configured timeout seconds.
Example approach (adjust units to match formatTimeDetailed):
- If effectiveStatus === 'failed' and original status ∈ {waiting, processing, building}, show formatTimeDetailed(getBuildTimeoutSeconds($regionalConsoleVariables)).
76-90: Map ‘waiting’ to a badge type for better visual signal.badgeTypeDeployment returns undefined for ‘waiting’. Consider returning 'warning' for all pending states.
export function badgeTypeDeployment(status: string) { switch (status) { case 'failed': return 'error'; case 'ready': return 'success'; - case 'building': + case 'building': return 'warning'; + case 'waiting': + return 'warning'; case 'processing': return undefined; default: return undefined; } }src/lib/helpers/buildTimeout.ts (3)
6-23: Inject ‘now’ for determinism and easier reactivity; add guards.Date.now() inside the helper hinders testing and SSR/hydration consistency, and makes UI reactivity awkward. Accept an optional nowMs parameter and guard invalid/negative elapsed.
-export function isBuildTimedOut( - createdAt: string, - status: string, - timeoutSeconds: number -): boolean { - if (!['waiting', 'processing', 'building'].includes(status)) { +const PENDING_STATUSES = new Set(['waiting', 'processing', 'building']); + +export function isBuildTimedOut( + createdAt: string, + status: string, + timeoutSeconds: number, + nowMs: number = Date.now() +): boolean { + if (!PENDING_STATUSES.has(status)) { return false; } if (!timeoutSeconds || timeoutSeconds <= 0) { return false; } - const created = new Date(createdAt); - const elapsedSeconds = Math.floor((Date.now() - created.getTime()) / 1000); + const created = new Date(createdAt); + const createdMs = created.getTime(); + if (Number.isNaN(createdMs)) return false; + const elapsedSeconds = Math.floor((nowMs - createdMs) / 1000); + if (elapsedSeconds < 0) return false; return elapsedSeconds > timeoutSeconds; }
28-37: Plumb through optional nowMs for consistency with isBuildTimedOut.-export function getEffectiveBuildStatus( - originalStatus: string, - createdAt: string, - timeoutSeconds: number -): string { - if (isBuildTimedOut(createdAt, originalStatus, timeoutSeconds)) { +export function getEffectiveBuildStatus( + originalStatus: string, + createdAt: string, + timeoutSeconds: number, + nowMs: number = Date.now() +): string { + if (isBuildTimedOut(createdAt, originalStatus, timeoutSeconds, nowMs)) { return 'failed'; } return originalStatus; }
45-49: Minor: be explicit about number parsing and types.Consider Number(...) with a strict check and typing the return as a branded number or at least narrowing ≥ 0.
- const timeout = parseInt(String(consoleVariables._APP_COMPUTE_BUILD_TIMEOUT), 10); - return isNaN(timeout) ? 0 : timeout; + const n = Number(consoleVariables._APP_COMPUTE_BUILD_TIMEOUT); + return Number.isFinite(n) && n > 0 ? Math.trunc(n) : 0;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/lib/helpers/buildTimeout.ts(1 hunks)src/routes/(console)/project-[region]-[project]/functions/function-[function]/deployment-[deployment]/+page.svelte(6 hunks)src/routes/(console)/project-[region]-[project]/sites/(components)/deploymentActionMenu.svelte(7 hunks)src/routes/(console)/project-[region]-[project]/sites/(components)/logsTimer.svelte(1 hunks)src/routes/(console)/project-[region]-[project]/sites/site-[site]/deployments/deployment-[deployment]/+page.svelte(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/routes/(console)/project-[region]-[project]/sites/site-[site]/deployments/deployment-[deployment]/+page.svelte
- src/routes/(console)/project-[region]-[project]/sites/(components)/logsTimer.svelte
- src/routes/(console)/project-[region]-[project]/sites/(components)/deploymentActionMenu.svelte
🧰 Additional context used
🧬 Code graph analysis (1)
src/lib/helpers/buildTimeout.ts (1)
src/routes/(console)/store.ts (1)
consoleVariables(9-12)
⏰ 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). (2)
- GitHub Check: build
- GitHub Check: e2e
🔇 Additional comments (1)
src/routes/(console)/project-[region]-[project]/functions/function-[function]/deployment-[deployment]/+page.svelte (1)
27-29: No action required—import is valid.The verification confirms that
regionalConsoleVariablesis correctly exported fromsrc/routes/(console)/project-[region]-[project]/store.ts(line 34), matching the import path and symbol name in the code. The symbol exists and no runtime import error will occur.
...roject-[region]-[project]/functions/function-[function]/deployment-[deployment]/+page.svelte
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (1)
src/lib/helpers/buildTimeout.ts:1
- Corrected spelling of 'mmoize' to 'memoize'.
import type { Models } from '@appwrite.io/console';
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ) | ||
| ) { | ||
| if (message.payload.status === 'ready') { | ||
| if (['ready', 'failed'].includes(message.payload.status)) { |
There was a problem hiding this comment.
The subscription handler now triggers invalidation for both 'ready' and 'failed' statuses, but this doesn't account for client-side timeout detection. If a build times out client-side (status becomes 'failed' via getEffectiveBuildStatus) but the server hasn't sent a status update, the subscription won't invalidate. Consider adding a client-side timeout check or periodic invalidation for in-progress builds.
src/lib/helpers/buildTimeout.ts
Outdated
| /** | ||
| * Checks if a build has exceeded the maximum build timeout duration | ||
| */ | ||
| export function isBuildTimedOut( |
There was a problem hiding this comment.
if not used anywhere else outside of this file, no need to export.
| getEffectiveBuildStatus( | ||
| data.deployment.status, | ||
| data.deployment.$createdAt, | ||
| getBuildTimeoutSeconds($regionalConsoleVariables) |
There was a problem hiding this comment.
if the 3rd param of getEffectiveBuildStatus is always calling getBuildTimeoutSeconds($regionalConsoleVariables), lets just inline that in then getEffectiveBuildStatus and just pass the variables.
src/routes/(console)/project-[region]-[project]/sites/(components)/logsTimer.svelte
Outdated
Show resolved
Hide resolved
…nts)/logsTimer.svelte Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/routes/(console)/project-[region]-[project]/functions/function-[function]/deployment-[deployment]/+page.svelte (1)
43-49: Timeout still never propagates to the pageThis is the same issue flagged earlier:
effectiveStatusis derived from static props, so it never recomputes after mount. The page footer buttons, logs badge, and timers therefore never transition to “failed” when the timeout expires. Wire in a ticking signal (e.g.,$stateclock bumped viasetIntervaland read inside the $derived) or consume a time-reactive status store sogetEffectiveBuildStatusactually runs over time.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/lib/helpers/buildTimeout.ts(1 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(6 hunks)src/routes/(console)/project-[region]-[project]/functions/function-[function]/table.svelte(5 hunks)src/routes/(console)/project-[region]-[project]/sites/(components)/deploymentActionMenu.svelte(7 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)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/routes/(console)/project-[region]-[project]/sites/site-[site]/deployments/table.svelte
- src/routes/(console)/project-[region]-[project]/sites/(components)/siteCard.svelte
- src/routes/(console)/project-[region]-[project]/functions/function-[function]/(components)/deploymentCard.svelte
- src/routes/(console)/project-[region]-[project]/sites/site-[site]/deploymentsOverview.svelte
- src/routes/(console)/project-[region]-[project]/sites/site-[site]/deployments/deployment-[deployment]/+page.svelte
- src/lib/helpers/buildTimeout.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-30T07:41:06.679Z
Learnt from: ItzNotABug
Repo: appwrite/console PR: 2425
File: src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte:454-468
Timestamp: 2025-09-30T07:41:06.679Z
Learning: In `src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte`, the column suggestions API (console.suggestColumns) has a maximum limit of 7 columns returned, which aligns with the initial placeholder count of 7 in customColumns.
Applied to files:
src/routes/(console)/project-[region]-[project]/functions/function-[function]/table.svelte
📚 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.svelte
| {@const effectiveStatus = getEffectiveBuildStatus( | ||
| deployment.status, | ||
| deployment.$createdAt, | ||
| $regionalConsoleVariables | ||
| )} |
There was a problem hiding this comment.
Row status never flips to “failed” after the timeout
This @const effectiveStatus = getEffectiveBuildStatus(...) only re-evaluates when the row re-renders, but nothing here reacts to the passage of time. Result: deployments that should time out remain “building,” so the badge, timer, and action buttons never transition. Please introduce a ticking dependency (e.g., a $state clock referenced in getEffectiveBuildStatus) or supply a pre-computed, time-reactive effectiveStatus from upstream so rows update once the timeout elapses.
🤖 Prompt for AI Agents
In
src/routes/(console)/project-[region]-[project]/functions/function-[function]/table.svelte
around lines 107–111, the computed effectiveStatus is only evaluated on row
re-render and never updates as time passes; modify the component to supply a
time-reactive dependency to getEffectiveBuildStatus (for example, create a
lightweight store like `now` or `state` that updates every second/minute and
reference `$now` in the call) or accept a pre-computed, reactive effectiveStatus
from the parent; ensure getEffectiveBuildStatus takes the current time (or reads
the reactive store) so the row re-evaluates when the timeout elapses and the UI
flips to “failed.”
| {@const effectiveStatus = getEffectiveBuildStatus( | ||
| deployment.status, | ||
| deployment.$createdAt, | ||
| $regionalConsoleVariables | ||
| )} |
There was a problem hiding this comment.
Menu never updates when the timeout is crossed
effectiveStatus is calculated once when this template renders. Because no reactive clock or prop changes fire afterward, a deployment that should time out continues to look “building,” so “Cancel” stays visible and “Delete” stays hidden indefinitely. Please drive this through a time-reactive status (e.g., pass one down from the table/card that ticks) or introduce a local ticking signal so getEffectiveBuildStatus actually re-runs as time advances.
| let effectiveStatus = $derived( | ||
| getEffectiveBuildStatus(deployment.status, deployment.$createdAt, $regionalConsoleVariables) | ||
| ); |
There was a problem hiding this comment.
Logs badge stays “building” forever once mounted
effectiveStatus is derived solely from deployment data and console variables; neither changes when the timeout threshold is crossed, so the derived value never flips to “failed.” The spinner view and badge therefore keep showing “building.” Add a ticking dependency (e.g., a $state timer updated every second and referenced inside the $derived) or feed this component an already time-reactive status so it re-computes as time passes.
🤖 Prompt for AI Agents
In
src/routes/(console)/project-[region]-[project]/sites/(components)/logs.svelte
around lines 43–45, the derived effectiveStatus only depends on deployment and
console variables so it never recomputes when the build timeout elapses; add a
ticking reactive dependency (e.g., create a small writable store or local $state
counter updated via setInterval on mount and cleared on destroy) and reference
that $state inside the $derived call (or pass a time-reactive value such as
Date.now() from that timer into getEffectiveBuildStatus) so the derived value
recomputes every second and flips from “building” to “failed” when the threshold
is crossed.
| let effectiveStatus = $derived( | ||
| getEffectiveBuildStatus(status, deployment.$createdAt, $regionalConsoleVariables) | ||
| ); |
There was a problem hiding this comment.
Timeout never triggers without a ticking dependency
effectiveStatus only depends on status, deployment.$createdAt, and $regionalConsoleVariables, none of which change after mount, so this $derived runs once. Builds that exceed the timeout remain stuck on “building/processing,” defeating the goal of this PR. Add a lightweight clock (e.g., a $state counter updated via setInterval that you reference inside the $derived) or pass in an already time-reactive effectiveStatus from the parent so the helper re-evaluates as time passes.
🤖 Prompt for AI Agents
In
src/routes/(console)/project-[region]-[project]/sites/(components)/logsTimer.svelte
around lines 12-14, the derived store `effectiveStatus` only depends on static
values so it never re-runs and timeouts never trigger; add a lightweight ticking
dependency (e.g., create a local writable `$tick` counter updated via
setInterval in onMount and cleared in onDestroy) and reference that tick inside
the $derived (or accept a time-reactive effectiveStatus prop from the parent) so
the derived recomputes as time passes and the timeout logic can fire.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/routes/(console)/project-[region]-[project]/functions/function-[function]/deployment-[deployment]/+page.svelte (1)
4-4:sdkmust be imported before use
onMountnow callssdk.forConsole.client.subscribe, butsdkisn’t imported, so the script fails to compile. Replace the obsoleterealtimeimport withsdk:-import { realtime } from '$lib/stores/sdk'; +import { sdk } from '$lib/stores/sdk';
♻️ Duplicate comments (2)
src/routes/(console)/project-[region]-[project]/functions/function-[function]/table.svelte (1)
88-92: Timeout still won’t propagate without a ticking dependency(Flagged earlier but still open.)
@const effectiveStatus = …only re-evaluates when the row re-renders, so timed-out builds stay “building/processing.”Reuse the fix from the sites table: introduce a shared
timeoutTick(setInterval+ cleanup) and reference it inside the@constso the status flips when the timeout elapses.+ import { onMount } from 'svelte'; @@ - let showRedeploy = $state(false); + let showRedeploy = $state(false); + let timeoutTick = $state(0); + + onMount(() => { + const timer = setInterval(() => timeoutTick++, 1_000); + return () => clearInterval(timer); + }); @@ - {@const effectiveStatus = getEffectiveBuildStatus( - deployment.status, - deployment.$createdAt, - $regionalConsoleVariables - )} + {@const effectiveStatus = + (timeoutTick, + getEffectiveBuildStatus( + deployment.status, + deployment.$createdAt, + $regionalConsoleVariables + ))}src/routes/(console)/project-[region]-[project]/functions/function-[function]/deployment-[deployment]/+page.svelte (1)
43-49: Timeout still can’t flip to “failed,” and the subscription never cleans up(Already noted in a previous review, but unchanged.) With the current
$derived, timeouts never surface; plus, the subscription leaks becauseunsubscribeis never invoked.Adopt the ticking dependency (and clear both timer + subscription on teardown):
@@ - let effectiveStatus = $derived( - getEffectiveBuildStatus( - data.deployment.status, - data.deployment.$createdAt, - $regionalConsoleVariables - ) - ); + let timeoutTick = $state(0); + let effectiveStatus = $derived(() => { + timeoutTick; + return getEffectiveBuildStatus( + data.deployment.status, + data.deployment.$createdAt, + $regionalConsoleVariables + ); + }); @@ - onMount(() => { - const unsubscribe = sdk.forConsole.client.subscribe<Models.Deployment>( - 'console', - (message) => { - if ( - message.events.includes( - `functions.${page.params.function}.deployments.${page.params.deployment}.update` - ) - ) { - if (['ready', 'failed'].includes(message.payload.status)) { - invalidate(Dependencies.DEPLOYMENT); - } - } - } - }); + onMount(() => { + const timer = setInterval(() => timeoutTick++, 1_000); + const unsubscribe = sdk.forConsole.client.subscribe<Models.Deployment>( + 'console', + (message) => { + if ( + message.events.includes( + `functions.${page.params.function}.deployments.${page.params.deployment}.update` + ) + ) { + if (['ready', 'failed'].includes(message.payload.status)) { + invalidate(Dependencies.DEPLOYMENT); + } + } + } + ); + return () => { + clearInterval(timer); + unsubscribe?.(); + }; });
📜 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/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(4 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(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-09-30T07:41:06.679Z
Learnt from: ItzNotABug
Repo: appwrite/console PR: 2425
File: src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte:454-468
Timestamp: 2025-09-30T07:41:06.679Z
Learning: In `src/routes/(console)/project-[region]-[project]/databases/database-[database]/(suggestions)/empty.svelte`, the column suggestions API (console.suggestColumns) has a maximum limit of 7 columns returned, which aligns with the initial placeholder count of 7 in customColumns.
Applied to files:
src/routes/(console)/project-[region]-[project]/functions/function-[function]/table.svelte
📚 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.svelte
📚 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
🪛 GitHub Actions: E2E
src/routes/(console)/project-[region]-[project]/functions/function-[function]/deployment-[deployment]/+page.svelte
[error] 1-1: pnpm run e2e failed. WebServer failed to start due to vite-plugin-svelte preprocessing error in +page.svelte: [svelte-preprocess] Encountered type error during preprocessing.
🪛 GitHub Actions: Tests
src/routes/(console)/project-[region]-[project]/functions/function-[function]/table.svelte
[error] 1-1: Preprocessing failed: Encountered type error (svelte-preprocess).
[warning] 65-73: </svelte:fragment> attempted to close an element that was not open (svelte).
src/routes/(console)/project-[region]-[project]/functions/function-[function]/deployment-[deployment]/+page.svelte
[error] 69-70: TS1005: ',' expected. (at 69:9) 69 });
[error] 70-70: TS1128: Declaration or statement expected. (at 70:5) });
src/routes/(console)/project-[region]-[project]/functions/function-[function]/table.svelte
Outdated
Show resolved
Hide resolved
| let effectiveStatus = $derived( | ||
| getEffectiveBuildStatus(deployment.status, deployment.$createdAt, $regionalConsoleVariables) | ||
| ); |
There was a problem hiding this comment.
Timeout never flips without a ticking dependency
$derived only re-evaluates when one of its tracked inputs changes. Because nothing here reacts to time, deployments that should time out stay “building” forever, defeating the PR’s goal.
Add a lightweight tick store, reference it in the derivation, and reuse the existing onMount to clear both the interval and the realtime subscription:
@@
- let effectiveStatus = $derived(
- getEffectiveBuildStatus(
- deployment.status,
- deployment.$createdAt,
- $regionalConsoleVariables
- )
- );
+ let timeoutTick = $state(0);
+ let effectiveStatus = $derived(() => {
+ timeoutTick;
+ return getEffectiveBuildStatus(
+ deployment.status,
+ deployment.$createdAt,
+ $regionalConsoleVariables
+ );
+ });
@@
- onMount(() => {
- return realtime.forConsole(page.params.region, 'console', async (response) => {
+ onMount(() => {
+ const timer = setInterval(() => timeoutTick++, 1_000);
+ const dispose = realtime.forConsole(page.params.region, 'console', async (response) => {
if (
response.events.includes(
`sites.${page.params.site}.deployments.${page.params.deployment}.update`
)
) {
await invalidate(Dependencies.DEPLOYMENT);
}
});
+ return () => {
+ clearInterval(timer);
+ dispose?.();
+ };
});📝 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.
| let effectiveStatus = $derived( | |
| getEffectiveBuildStatus(deployment.status, deployment.$createdAt, $regionalConsoleVariables) | |
| ); | |
| let timeoutTick = $state(0); | |
| let effectiveStatus = $derived(() => { | |
| timeoutTick; | |
| return getEffectiveBuildStatus( | |
| deployment.status, | |
| deployment.$createdAt, | |
| $regionalConsoleVariables | |
| ); | |
| }); | |
| onMount(() => { | |
| const timer = setInterval(() => timeoutTick++, 1_000); | |
| const dispose = realtime.forConsole(page.params.region, 'console', async (response) => { | |
| if ( | |
| response.events.includes( | |
| `sites.${page.params.site}.deployments.${page.params.deployment}.update` | |
| ) | |
| ) { | |
| await invalidate(Dependencies.DEPLOYMENT); | |
| } | |
| }); | |
| return () => { | |
| clearInterval(timer); | |
| dispose?.(); | |
| }; | |
| }); |
🤖 Prompt for AI Agents
In
src/routes/(console)/project-[region]-[project]/sites/site-[site]/deployments/deployment-[deployment]/+page.svelte
around lines 29-31, the derived store for effectiveStatus never re-runs because
it has no time/tick dependency; add a lightweight tick store (e.g., writable
number updated on a setInterval) and include it as a tracked input to the
$derived call so getEffectiveBuildStatus is re-evaluated periodically; in the
existing onMount cleanup callback clear the interval and also
unsubscribe/cleanup the realtime subscription so both the timer and the realtime
listener are properly disposed.
There was a problem hiding this comment.
On refresh or coming back, it would work right?
There was a problem hiding this comment.
@HarshMN2345, yes, you're absolutely right! On page refresh or when navigating back to the page, the component remounts and getEffectiveBuildStatus will be called again with the current time, so the timeout would be correctly detected and displayed.
The trade-off is:
Current implementation (without tick):
- ✅ Timeout works on refresh / navigation
- ❌ User watching a live build won't see status flip to "failed" automatically when timeout is reached—they'd need to refresh
With tick (as suggested):
- ✅ Timeout flips automatically while user watches
⚠️ Additional setInterval overhead (though minimal—just incrementing a counter every second)
Given your PR goal is to "prevent stuck build UI", the current approach does achieve that for most scenarios (users returning to see the status). If you expect users to commonly watch builds in real-time and want them to see the timeout without manual refresh, the tick approach would provide better UX.
What's your preference based on your typical user workflow?
src/routes/(console)/project-[region]-[project]/sites/site-[site]/deployments/table.svelte
Outdated
Show resolved
Hide resolved
| {@const effectiveStatus = getEffectiveBuildStatus( | ||
| deployment.status, | ||
| deployment.$createdAt, | ||
| $regionalConsoleVariables | ||
| )} |
There was a problem hiding this comment.
Rows never time out without a ticking dependency
{@const effectiveStatus = ...} is evaluated only when the row re-renders. Because nothing here changes over time, builds that should time out stay “building,” so badges/actions never update.
Add a small timer in the script (don’t forget to import { onMount } from 'svelte';), use it to trigger re-computation, and reference it in the @const expression:
@@
- import { Layout, Status, Table } from '@appwrite.io/pink-svelte';
+ import { Layout, Status, Table } from '@appwrite.io/pink-svelte';
+ import { onMount } from 'svelte';
@@
- let showCancel = $state(false);
+ let showCancel = $state(false);
+ let timeoutTick = $state(0);
+
+ onMount(() => {
+ const timer = setInterval(() => timeoutTick++, 1_000);
+ return () => clearInterval(timer);
+ });
@@
- {@const effectiveStatus = getEffectiveBuildStatus(
- deployment.status,
- deployment.$createdAt,
- $regionalConsoleVariables
- )}
+ {@const effectiveStatus =
+ (timeoutTick,
+ getEffectiveBuildStatus(
+ deployment.status,
+ deployment.$createdAt,
+ $regionalConsoleVariables
+ ))}That keeps the expression pure while forcing it to re-evaluate once the timeout threshold is crossed.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
src/routes/(console)/project-[region]-[project]/sites/site-[site]/deployments/table.svelte
around lines 75-79, the computed effectiveStatus never re-evaluates over time
because the @const expression has no time-based dependency; add a small ticking
dependency in the script: import onMount (and onDestroy) from 'svelte', create a
local tick variable (e.g. a number or boolean) and start a timer in onMount that
updates tick at a suitable interval (or schedules a timeout to fire when the
timeout threshold is expected), clear the timer in onDestroy, and reference that
tick variable inside the {@const effectiveStatus = ...} expression so the value
is pure but re-computes when time passes.
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 (1)
src/routes/(console)/project-[region]-[project]/functions/function-[function]/deployment-[deployment]/+page.svelte (1)
133-133: Inconsistent status check bypasses timeout logic.Lines 133 and 141 still reference
data.deployment.statusinstead ofeffectiveStatus, which bypasses the timeout handling. If a deployment times out client-side, these buttons will remain visible based on the server's stale status, creating inconsistent UI behavior with the rest of the page.Apply this diff to use effectiveStatus consistently:
-{#if data.func.deploymentId === data.deployment.$id && data.deployment.status === 'ready'} +{#if data.func.deploymentId === data.deployment.$id && effectiveStatus === 'ready'} <Button href={`${base}/project-${page.params.region}-${page.params.project}/functions/function-${page.params.function}/executions/execute-function`} disabled={isCloud && $readOnly}> Execute </Button> {/if} -{#if data.func.deploymentId !== data.deployment.$id && data.deployment.status === 'ready'} +{#if data.func.deploymentId !== data.deployment.$id && effectiveStatus === 'ready'} <Button disabled={data.activeDeployment} on:click={() => { showActivate = true; }}>Activate</Button> {/if}Also applies to: 141-141
♻️ Duplicate comments (3)
src/routes/(console)/project-[region]-[project]/functions/function-[function]/deployment-[deployment]/+page.svelte (2)
55-71: Subscription won't invalidate on client-side timeout.The subscription handler triggers invalidation only when the server sends 'ready' or 'failed' status updates. If a build times out client-side (effectiveStatus becomes 'failed' via getEffectiveBuildStatus) but the server hasn't sent a status update, the subscription won't invalidate and stale data persists.
43-49: Effective status won't flip to "failed" over time — make it time‑reactive.$derived doesn't track time; without a ticking dependency, effectiveStatus never updates after mount. This defeats the timeout goal and the UI can stay "building/processing" indefinitely.
src/routes/(console)/project-[region]-[project]/sites/site-[site]/deployments/table.svelte (1)
76-80: Timeout logic is non-functional without a ticking dependency.The
{@const effectiveStatus}expression is only evaluated when the row re-renders. Since there's no time-based state variable that triggers re-renders, builds will never transition to "timeout" status as time passes—they'll remain stuck showing "building" indefinitely.Add a ticking state variable in the script section and reference it in this expression:
+ import { onMount } from 'svelte'; + let showCancel = $state(false); + let tick = $state(0); + + onMount(() => { + const interval = setInterval(() => tick++, 1_000); + return () => clearInterval(interval); + });Then reference the tick in the const expression to force re-evaluation:
- {@const effectiveStatus = getEffectiveBuildStatus( + {@const effectiveStatus = (tick, getEffectiveBuildStatus( deployment.status, deployment.$createdAt, $regionalConsoleVariables - )} + ))}
🧹 Nitpick comments (1)
src/routes/(console)/project-[region]-[project]/functions/function-[function]/deployment-[deployment]/+page.svelte (1)
43-49: Consider inlining getBuildTimeoutSeconds as suggested.A past review suggested simplifying the
getEffectiveBuildStatusAPI by passingregionalConsoleVariablesdirectly and inlining thegetBuildTimeoutSecondscall within the helper function, rather than requiring callers to pass the computed timeout seconds.Based on past review comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/routes/(console)/project-[region]-[project]/functions/function-[function]/deployment-[deployment]/+page.svelte(6 hunks)src/routes/(console)/project-[region]-[project]/functions/function-[function]/table.svelte(5 hunks)src/routes/(console)/project-[region]-[project]/sites/site-[site]/deployments/table.svelte(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/routes/(console)/project-[region]-[project]/functions/function-[function]/table.svelte
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-09-08T13:20:47.308Z
Learnt from: ItzNotABug
Repo: appwrite/console PR: 2316
File: src/routes/(console)/project-[region]-[project]/functions/create-function/deploy/+page.svelte:29-29
Timestamp: 2025-09-08T13:20:47.308Z
Learning: The Form.svelte component in the Appwrite console creates a FormContext with isSubmitting as writable(false) and expects consumers to work with Svelte writable stores, not plain booleans.
Applied to files:
src/routes/(console)/project-[region]-[project]/functions/function-[function]/deployment-[deployment]/+page.svelte
📚 Learning: 2025-10-13T05:13:54.542Z
Learnt from: ItzNotABug
Repo: appwrite/console PR: 2413
File: src/routes/(console)/project-[region]-[project]/databases/table.svelte:33-39
Timestamp: 2025-10-13T05:13:54.542Z
Learning: In Svelte 5, `import { page } from '$app/state'` provides a reactive state proxy that can be accessed directly (e.g., `page.params`), unlike the older `import { page } from '$app/stores'` which returns a readable store requiring the `$page` syntax for auto-subscription in components.
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
📚 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.svelte
⏰ 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). (2)
- GitHub Check: e2e
- GitHub Check: build
🔇 Additional comments (3)
src/routes/(console)/project-[region]-[project]/functions/function-[function]/deployment-[deployment]/+page.svelte (1)
93-93: LGTM! Consistent use of effectiveStatus for UI rendering.These segments correctly reference effectiveStatus to drive cancel button visibility, badge rendering, and logs timer display, aligning with the timeout handling feature.
Also applies to: 155-157, 170-170
src/routes/(console)/project-[region]-[project]/sites/site-[site]/deployments/table.svelte (2)
122-129: No changes required—DeploymentActionMenu already computes and useseffectiveStatusinternally.The component already correctly computes
effectiveStatusinternally (line 56-60) and uses it for all action visibility decisions (Activate, Cancel, Delete, Download). PassingeffectiveStatusas a prop would be redundant. The code properly handles timed-out builds as-is.Likely an incorrect or invalid review comment.
104-110: 'Waiting' is a valid deployment status; clarify the timer concern.The
'waiting'status on line 104 is valid and correctly documented. It appears in the deployment store schema, status converter logic, and buildTimeout helper—showing "-" for builds that haven't started is correct.The timer concern may point to a real reactivity issue (timer not stopping until the server updates deployment status), but this depends on the separate "ticking dependency issue" you reference. Clarify whether this is a known limitation or a blocker for this change.
...roject-[region]-[project]/functions/function-[function]/deployment-[deployment]/+page.svelte
Outdated
Show resolved
Hide resolved
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
Chores