-
Notifications
You must be signed in to change notification settings - Fork 22
Sveltekit form actions take 2 #76
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?
Sveltekit form actions take 2 #76
Conversation
|
@alexvdvalk is attempting to deploy a commit to the Directus Team on Vercel. A member of the Team first needs to authorize it. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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.
Pull request overview
This PR refactors form submissions from client-side to server-side processing using SvelteKit form actions, addressing a critical security concern by preventing the API key from leaking to the frontend.
Key Changes:
- Migrated form submission from client-side to SvelteKit server actions
- Replaced
PUBLIC_DIRECTUS_TOKENandPUBLIC_DIRECTUS_FORM_TOKENwith server-onlyDIRECTUS_SERVER_TOKEN - Updated UI components to support Svelte 5 with file input handling
- Updated dependencies including SvelteKit (2.36.1 → 2.49.0), Svelte (5.38.2 → 5.44.0), and bits-ui (2.9.4 → 1.8.0)
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
cms/sveltekit/src/routes/[...permalink]/+page.server.ts |
Adds form action endpoint for handling form submissions |
cms/sveltekit/src/routes/+layout.server.ts |
Minor formatting changes (empty lines added) |
cms/sveltekit/src/lib/types/directus-schema.ts |
Auto-generated type updates with formatting changes |
cms/sveltekit/src/lib/directus/generateDirectusTypes.ts |
Updates to use server-only token instead of public token |
cms/sveltekit/src/lib/directus/forms.ts |
Refactored form submission to use server-side processing with SvelteKit form actions |
cms/sveltekit/src/lib/directus/fetchers-forms.ts |
New file for server-side form field fetching |
cms/sveltekit/src/lib/components/ui/input/input.svelte |
Migrates to Svelte 5 with file input support |
cms/sveltekit/src/lib/components/ui/input/index.ts |
Simplifies exports by removing event type definitions |
cms/sveltekit/src/lib/components/forms/formBuilderTypes.ts |
New type definitions file for form builder props |
cms/sveltekit/src/lib/components/forms/fields/FileUploadField.svelte |
Adds name attribute and required validation |
cms/sveltekit/src/lib/components/forms/FormField.svelte |
Updates to use consistent field naming with fieldName variable |
cms/sveltekit/src/lib/components/forms/FormBuilder.svelte |
Refactored to delegate submission handling to DynamicForm |
cms/sveltekit/src/lib/components/forms/DynamicForm.svelte |
Complete refactor to use SvelteKit form actions with enhance |
cms/sveltekit/pnpm-lock.yaml |
Updates dependency versions and resolution changes |
cms/sveltekit/package.json |
Updates SvelteKit, Svelte, and bits-ui versions |
cms/sveltekit/components.json |
Updates shadcn-svelte configuration |
cms/sveltekit/.env.example |
Consolidates tokens to single server-side token |
Files not reviewed (1)
- cms/sveltekit/pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| </Form.Control> | ||
| <Form.Description>{field.help}</Form.Description> | ||
|
|
||
| <!-- When this renders, it causes the form fields to be out of line --> |
Copilot
AI
Dec 10, 2025
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.
[nitpick] The comment here is misleading. The issue isn't specifically related to form fields being "out of line" - this is the standard location for error messages in a form. Consider updating this comment to be more specific about what issue you're referring to, or remove it if the issue has been resolved.
| <!-- When this renders, it causes the form fields to be out of line --> |
| )} | ||
| type="file" | ||
| bind:files | ||
| bind:value |
Copilot
AI
Dec 10, 2025
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.
Binding both files and value on the same file input (lines 31-32) is unnecessary and could cause issues. For file inputs, only the files binding is needed. The value property on file inputs is read-only and typically just contains the filename. Consider removing the bind:value on line 32.
| bind:value |
| bind:checked={$formData[fieldName!]} | ||
| required={!!field.required} | ||
| /> | ||
| <Label for={field.name}>{field.label}</Label> |
Copilot
AI
Dec 10, 2025
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.
The for attribute should reference fieldName instead of field.name for consistency with the rest of the component where fieldName is used (e.g., lines 77, 85, 93).
| <Label for={field.name}>{field.label}</Label> | |
| <Label for={fieldName}>{field.label}</Label> |
| </script> | ||
|
|
||
| <Input type="file" id={name} onchange={onFileChange} /> | ||
| <Input type="file" id={name} {name} onchange={onFileChange} required={!!formData.required} /> |
Copilot
AI
Dec 10, 2025
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.
The required attribute references formData.required but formData is the form data store (from destructuring form.form), not the field configuration. This will always be falsy since form data doesn't have a required property. The field's required value should be passed as a prop to this component.
| const payload: Omit<FormSubmission, 'id'> = { | ||
| form: formId, | ||
| values: submissionValues | ||
| values: submissionValues as FormSubmissionValue[] // this shiould be <omit<FormSubmissionValue, 'id'>>[] |
Copilot
AI
Dec 10, 2025
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.
There's a typo in the comment. "shiould" should be "should".
| values: submissionValues as FormSubmissionValue[] // this shiould be <omit<FormSubmissionValue, 'id'>>[] | |
| values: submissionValues as FormSubmissionValue[] // this should be <omit<FormSubmissionValue, 'id'>>[] |
| await submitForm(formId, form.fields as FormField[], formData); | ||
|
|
||
| return { success: true }; |
Copilot
AI
Dec 10, 2025
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.
The error is caught and logged but not returned to the client. The function returns { success: true } on line 24 regardless of whether the submission succeeds or fails. This means clients will always receive a success response even when form submission fails. Consider wrapping the submitForm call in a try-catch block and returning appropriate error responses.
| await submitForm(formId, form.fields as FormField[], formData); | |
| return { success: true }; | |
| try { | |
| await submitForm(formId, form.fields as FormField[], formData); | |
| return { success: true }; | |
| } catch (error) { | |
| console.error('Error in submitFormAction:', error); | |
| return fail(500, { error: error instanceof Error ? error.message : 'Failed to submit form' }); | |
| } |
| "@types/node": "^22.17.2", | ||
| "autoprefixer": "^10.4.21", | ||
| "bits-ui": "^2.9.4", | ||
| "bits-ui": "^1.8.0", |
Copilot
AI
Dec 10, 2025
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.
Downgrading bits-ui from ^2.9.4 to ^1.8.0 is a major version downgrade. This could introduce breaking changes or remove features that may be in use elsewhere in the codebase. Ensure this downgrade is intentional and that all components using bits-ui are compatible with version 1.8.0.
| "bits-ui": "^1.8.0", | |
| "bits-ui": "^2.9.4", |
| bind:value={$formData[field.name!]} | ||
| type={field.validation?.includes('email') ? 'email' : 'text'} | ||
| /> | ||
| {:else if field.type === 'textarea'} | ||
| <Textarea | ||
| {...props} | ||
| placeholder={field.placeholder || ''} | ||
| name={field.name || ''} | ||
| bind:value={$formData[field.name!]} | ||
| name={fieldName} | ||
| bind:value={$formData[fieldName!]} |
Copilot
AI
Dec 10, 2025
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.
Inconsistent field name variable usage. Line 86 uses $formData[fieldName!] while line 78 uses $formData[field.name!]. For consistency and correctness, all instances should use fieldName (defined at the top of the component) rather than field.name.
| values: submissionValues as FormSubmissionValue[] // this shiould be <omit<FormSubmissionValue, 'id'>>[] | ||
| }; | ||
|
|
||
| console.log("Form Payload", payload); |
Copilot
AI
Dec 10, 2025
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.
This console.log statement should be removed before merging to production. Debug logging can expose sensitive form data and clutters production logs.
| console.log("Form Payload", payload); |
| class="flex flex-wrap gap-4" | ||
| {onsubmit} | ||
| return async ({ result }) => { | ||
| console.log('formProp', formProp); |
Copilot
AI
Dec 10, 2025
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.
This console.log statement should be removed before merging to production. It can expose form configuration data and clutters production logs.
| console.log('formProp', formProp); |
This is a big but neccessary one.
I've locked down the API key from potentially leaking onto the front end.
And because of this, i had to refactor the form submissions to use a sveltekim implementation - https://svelte.dev/docs/kit/form-actions
Forms now get submitted to the sveltekit server and are then further processed and send to Directus API. This is a different implenentation to the Nextjs start but should give better compatibility with non-js environemtns.
Still need to optimize and test. for non-js environments