fix: handle missing deploymentId gracefully #2626
Conversation
Console (appwrite/console)Project ID: Tip Sites auto-generate unique domains with the pattern https://randomstring.appwrite.network |
WalkthroughRefactors deployment retrieval in three route files by adding a Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/routes/(console)/project-[region]-[project]/functions/function-[function]/+page.ts (1)
18-31: Graceful handling of missing function deployment looks good; consider small cleanups.The new
activeDeploymentlogic correctly guards ondata.function.deploymentIdand safely falls back tonullon any retrieval error, which aligns with the PR’s goal of not breaking the page when the deployment is missing.Two minor follow-ups you might consider:
- Since the caught
erroris not used, some TS/lint configs will complain. If you intend to ignore all errors here, either drop the binding (catch { ... }) or rename to_errorto satisfy lint rules.- You can avoid repeating
sdk.forProject(params.region, params.project)and slightly simplify the code by caching the project SDK once and reusing it for bothgetDeploymentandlistDeployments:- const parsedQueries = queryParamToMap(query || '[]'); - queries.set(parsedQueries); - let activeDeployment = null; + const parsedQueries = queryParamToMap(query || '[]'); + queries.set(parsedQueries); + const projectSdk = sdk.forProject(params.region, params.project); + let activeDeployment = null; @@ - activeDeployment = await sdk - .forProject(params.region, params.project) - .functions.getDeployment({ + activeDeployment = await projectSdk.functions.getDeployment({ functionId: params.function, deploymentId: data.function.deploymentId }); - } catch (error) { + } catch { // active deployment with the requested ID could not be found activeDeployment = null; } @@ - deploymentList: await sdk - .forProject(params.region, params.project) - .functions.listDeployments({ + deploymentList: await projectSdk.functions.listDeployments({These are non-blocking and mainly for cleanliness and minor reuse.
Also applies to: 39-42
src/routes/(console)/project-[region]-[project]/sites/site-[site]/+page.ts (1)
49-59: Deployment fetch is robust; tighten error handling and guardhasProdReadyDeployments.The guarded
deploymentlookup behaves as intended: it only runs when bothdeploymentList.totalandsite.deploymentIdexist, and failures are handled by falling back tonullso the rest of the page data is still usable.A couple of small improvements you might want to apply:
- The caught
erroris unused; if you don’t plan to log or branch on it, you can simplify the block and avoid potential lint issues:- } catch (error) { + } catch { // active deployment with the requested ID could not be found deployment = null; }
hasProdReadyDeploymentscurrently calls.lengthon the result ofprodReadyDeployments?.deployments?.filter(...). If eitherprodReadyDeploymentsor.deploymentsis undefined, this expression will throw at runtime (this is pre-existing, but worth fixing while you’re here). You can safely default to an empty array:- hasProdReadyDeployments: - prodReadyDeployments?.deployments?.filter((d) => d?.$id !== deployment?.$id).length > 0 + hasProdReadyDeployments: + (prodReadyDeployments?.deployments ?? []) + .filter((d) => d?.$id !== deployment?.$id).length > 0This keeps the semantics the same while making the code more defensive.
Also applies to: 61-69
src/routes/(console)/project-[region]-[project]/sites/site-[site]/deployments/+page.ts (1)
40-50: Consistent activeDeployment handling; consider minor refactor and catch cleanup.The
activeDeploymentcomputation mirrors the new pattern in other routes: it’s gated on bothsite.deploymentIdanddeploymentList.totaland safely degrades tonullon any error, which should handle missing/removed deployments without failing the page.Two small tidy-ups to consider:
- As in the other files, the caught
erroris unused. If intentional, you can remove the binding:- } catch (error) { + } catch { // active deployment with the requested ID could not be found activeDeployment = null; }
- For consistency and to avoid repeating
sdk.forProject(params.region, params.project)in multiple places in this file, you could cache it once near the top ofload(similar to the diff suggested in the functions page) and callprojectSdk.sites.getDeployment(...)/projectSdk.sites.listDeployments(...).These are non-blocking and mainly improve readability and maintainability.
Also applies to: 52-58
📜 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]/+page.ts(1 hunks)src/routes/(console)/project-[region]-[project]/sites/site-[site]/+page.ts(1 hunks)src/routes/(console)/project-[region]-[project]/sites/site-[site]/deployments/+page.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/routes/(console)/project-[region]-[project]/sites/site-[site]/deployments/+page.ts (1)
src/lib/stores/sdk.ts (1)
sdk(171-194)
src/routes/(console)/project-[region]-[project]/sites/site-[site]/+page.ts (2)
src/routes/(console)/project-[region]-[project]/functions/function-[function]/store.ts (1)
deploymentList(8-11)src/lib/stores/sdk.ts (1)
sdk(171-194)
src/routes/(console)/project-[region]-[project]/functions/function-[function]/+page.ts (1)
src/lib/stores/sdk.ts (1)
sdk(171-194)
⏰ 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
|
|
||
| const parsedQueries = queryParamToMap(query || '[]'); | ||
| queries.set(parsedQueries); | ||
| let activeDeployment = null; |
There was a problem hiding this comment.
lets add the variable type as well, just for visibility.
| sdk.forProject(params.region, params.project).vcs.listInstallations() | ||
| ]); | ||
|
|
||
| let activeDeployment = null; |
| .forProject(params.region, params.project) | ||
| .sites.getDeployment({ siteId: params.site, deploymentId: site.deploymentId }) | ||
| : null; | ||
| let deployment = null; |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/routes/(console)/project-[region]-[project]/sites/site-[site]/deployments/+page.ts (1)
1-1: GuardedactiveDeploymentfetch matches PR goal; consider optional diagnosticsThe precomputed
activeDeployment: Models.Deployment | nullwith a try/catch cleanly handles missing or invalidsite.deploymentIdby falling back tonull, while keeping the rest of the page data intact. If you later need to debug non‑404 failures, you might add lightweight logging or narrow the catch to known “not found” cases, but the current behavior is fine for graceful degradation.Also applies to: 40-50, 57-57
src/routes/(console)/project-[region]-[project]/sites/site-[site]/+page.ts (1)
3-3: Sitedeploymentretrieval now degrades gracefully on missing IDsThe explicit
deployment: Models.Deployment | nullplus guardedsites.getDeploymentcall correctly shields this route from stale or missingsite.deploymentIdwhile preserving existing uses ofdeployment(includinghasProdReadyDeployments). As with the deployments page, you might optionally log inside the catch or specialize for “not found” errors if distinguishing backend issues ever becomes important.Also applies to: 49-59
📜 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]/+page.ts(2 hunks)src/routes/(console)/project-[region]-[project]/sites/site-[site]/+page.ts(2 hunks)src/routes/(console)/project-[region]-[project]/sites/site-[site]/deployments/+page.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ItzNotABug
Repo: appwrite/console PR: 2316
File: src/routes/(console)/project-[region]-[project]/functions/create-function/deploy/+page.svelte:95-103
Timestamp: 2025-09-08T13:21:53.793Z
Learning: In Appwrite's one-click deployment flows, Git tag validation should be lenient with fallbacks (like '1.0.0') rather than strict validation that blocks deployments, since the source code retrieval from the repository is more important than having proper version tags for the deployment to succeed.
🧬 Code graph analysis (3)
src/routes/(console)/project-[region]-[project]/functions/function-[function]/+page.ts (1)
src/lib/stores/sdk.ts (1)
sdk(171-194)
src/routes/(console)/project-[region]-[project]/sites/site-[site]/+page.ts (2)
src/routes/(console)/project-[region]-[project]/functions/function-[function]/store.ts (1)
deploymentList(8-11)src/lib/stores/sdk.ts (1)
sdk(171-194)
src/routes/(console)/project-[region]-[project]/sites/site-[site]/deployments/+page.ts (2)
src/routes/(console)/project-[region]-[project]/functions/function-[function]/store.ts (1)
deploymentList(8-11)src/lib/stores/sdk.ts (1)
sdk(171-194)
⏰ 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 (1)
src/routes/(console)/project-[region]-[project]/functions/function-[function]/+page.ts (1)
1-1: FunctionactiveDeploymenthandling is consistent and robustTyping
activeDeploymentasModels.Deployment | nulland guarding thefunctions.getDeploymentcall behinddata.function.deploymentIdwith a try/catch gives the functions page the same graceful behavior as the site pages when a deployment is missing or deleted, without changing the returned data shape.Also applies to: 18-31, 38-38
What does this PR do?
(Provide a description of what this PR does.)
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?
(Write your answer here.)
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.
✏️ Tip: You can customize this high-level summary in your review settings.