-
Notifications
You must be signed in to change notification settings - Fork 197
Improve UX for manual sites & functions deployment #2523
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -23,6 +23,7 @@ | |||||||||||||||||
| import { isCloud } from '$lib/system'; | ||||||||||||||||||
| import { humanFileSize } from '$lib/helpers/sizeConvertion'; | ||||||||||||||||||
| import { currentPlan } from '$lib/stores/organization'; | ||||||||||||||||||
| import { uploader } from '$lib/stores/uploader'; | ||||||||||||||||||
| export let data; | ||||||||||||||||||
|
|
@@ -61,8 +62,10 @@ | |||||||||||||||||
| let specification = specificationOptions[0].value; | ||||||||||||||||||
| async function create() { | ||||||||||||||||||
| let func: Models.Function | null = null; | ||||||||||||||||||
| try { | ||||||||||||||||||
| const func = await sdk | ||||||||||||||||||
| func = await sdk | ||||||||||||||||||
| .forProject(page.params.region, page.params.project) | ||||||||||||||||||
| .functions.create({ | ||||||||||||||||||
| functionId: id || ID.unique(), | ||||||||||||||||||
|
|
@@ -92,25 +95,37 @@ | |||||||||||||||||
| ); | ||||||||||||||||||
| await Promise.all(promises); | ||||||||||||||||||
| await sdk | ||||||||||||||||||
| .forProject(page.params.region, page.params.project) | ||||||||||||||||||
| .functions.createDeployment({ | ||||||||||||||||||
| functionId: func.$id, | ||||||||||||||||||
| code: files[0], | ||||||||||||||||||
| activate: true | ||||||||||||||||||
| }); | ||||||||||||||||||
| const promise = uploader.uploadFunctionDeployment({ | ||||||||||||||||||
| functionId: func.$id, | ||||||||||||||||||
| code: files[0], | ||||||||||||||||||
| }); | ||||||||||||||||||
| addNotification({ | ||||||||||||||||||
| message: 'Deployment upload started', | ||||||||||||||||||
| type: 'success' | ||||||||||||||||||
| }); | ||||||||||||||||||
| trackEvent(Submit.FunctionCreate, { | ||||||||||||||||||
| source: 'repository', | ||||||||||||||||||
| runtime: runtime | ||||||||||||||||||
| }); | ||||||||||||||||||
|
Comment on lines
105
to
108
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Analytics source seems wrong for manual upload This page is “manual” but tracks - trackEvent(Submit.FunctionCreate, {
- source: 'repository',
- runtime: runtime
- });
+ trackEvent(Submit.FunctionCreate, {
+ source: 'manual',
+ runtime
+ });📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||
| await goto( | ||||||||||||||||||
| `${base}/project-${page.params.region}-${page.params.project}/functions/function-${func.$id}` | ||||||||||||||||||
| ); | ||||||||||||||||||
| await promise; | ||||||||||||||||||
| const upload = $uploader.files.find((f) => f.resourceId === func.$id); | ||||||||||||||||||
| if (upload?.status === 'success') { | ||||||||||||||||||
| const deploymentId = upload.$id; | ||||||||||||||||||
| await goto( | ||||||||||||||||||
| `${base}/project-${page.params.region}-${page.params.project}/functions/function-${func.$id}/deployment-${deploymentId}` | ||||||||||||||||||
| ); | ||||||||||||||||||
| } | ||||||||||||||||||
| invalidate(Dependencies.FUNCTION); | ||||||||||||||||||
| } catch (e) { | ||||||||||||||||||
| const upload = $uploader.files.find((f) => f.resourceId === func?.$id); | ||||||||||||||||||
| if (upload) { | ||||||||||||||||||
| uploader.removeFromQueue(upload.$id); | ||||||||||||||||||
| } | ||||||||||||||||||
| addNotification({ | ||||||||||||||||||
| type: 'error', | ||||||||||||||||||
| message: e.message | ||||||||||||||||||
|
|
||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,15 +6,14 @@ | |
| import { Button } from '$lib/elements/forms'; | ||
| import { InvalidFileType, removeFile } from '$lib/helpers/files'; | ||
| import { addNotification } from '$lib/stores/notifications'; | ||
| import { sdk } from '$lib/stores/sdk'; | ||
| import { IconInfo } from '@appwrite.io/pink-icons-svelte'; | ||
| import { Icon, Layout, Tooltip, Typography, Upload } from '@appwrite.io/pink-svelte'; | ||
| import { func } from '../store'; | ||
| import { page } from '$app/state'; | ||
| import { consoleVariables } from '$routes/(console)/store'; | ||
| import { currentPlan } from '$lib/stores/organization'; | ||
| import { isCloud } from '$lib/system'; | ||
| import { humanFileSize } from '$lib/helpers/sizeConvertion'; | ||
| import { uploader } from '$lib/stores/uploader'; | ||
| export let show = false; | ||
|
|
@@ -30,16 +29,13 @@ | |
| async function create() { | ||
| try { | ||
| await sdk | ||
| .forProject(page.params.region, page.params.project) | ||
| .functions.createDeployment({ | ||
| functionId: $func.$id, | ||
| code: files[0], | ||
| activate: true | ||
| }); | ||
| await invalidate(Dependencies.DEPLOYMENTS); | ||
| files = undefined; | ||
| uploader.uploadFunctionDeployment({ | ||
| functionId: $func.$id, | ||
| code: files[0], | ||
| }) | ||
| show = false; | ||
| files = undefined; | ||
| await invalidate(Dependencies.DEPLOYMENTS); | ||
| trackEvent(Submit.DeploymentCreate); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Make upload non‑blocking but safe; fix success message; add semicolon
- uploader.uploadFunctionDeployment({
- functionId: $func.$id,
- code: files[0],
- })
+ void uploader
+ .uploadFunctionDeployment({
+ functionId: $func.$id,
+ code: files[0],
+ })
+ .catch((e) => trackError(e, Submit.DeploymentCreate));
+ ;
show = false;
files = undefined;
await invalidate(Dependencies.DEPLOYMENTS);
- trackEvent(Submit.DeploymentCreate);
+ trackEvent(Submit.DeploymentCreate);
addNotification({
type: 'success',
- message: 'Deployment created successfully'
+ message: 'Deployment upload started'
});Also applies to: 41-43 🤖 Prompt for AI Agents |
||
| addNotification({ | ||
| type: 'success', | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
Consider: Potential race between onProgress and final status update.
Lines 158-161 set
status: 'success'whenprogress === 100, then lines 164-167 set it again after the promise resolves. If an error occurs between progress=100 and promise resolution, the status may incorrectly show 'success'.However, if
createDeploymentthrows on failure, the try-catch in the caller will handle it (though there's no try-catch here). Consider whether progress=100 truly indicates success or just upload completion.Check if the SDK's
createDeploymentcan fail afterprogress === 100:🌐 Web query:
💡 Result:
Short answer: No — progress = 100 only means the upload finished from the client side. The deployment/build stage runs afterwards and can still fail. Appwrite exposes separate deployment/build status (ready/failed/in‑progress) and error codes for build failures, and you must check the deployment status (or API response) after upload to confirm success. [1][2][3]
Sources:
Fix premature success status update when progress reaches 100%.
The concern is valid: setting
status: 'success'at line 160 whenprogress === 100is incorrect. Appwrite's upload progress only indicates client-side upload completion; the deployment/build runs separately and can still fail afterwards. Remove the prematurestatus: 'success'assignment from theonProgresshandler (line 160). Only setstatus: 'success'after confirming the deployment actually succeeded—either by checking the returned deployment object's status field or by verifying the API response indicates a ready/successful state.🤖 Prompt for AI Agents