refactor: move post-form validation logic to onUpdated event #1487
Conversation
This aims to centralize all post-valid behaviours, e.g. redirects, closing modals...
WalkthroughThis pull request refactors multiple form-related components and utilities. In the main form component, a new Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant SF as SuperForm Component
participant F as _form (superForm)
participant A as Action Handler
participant S as Server API
U->>SF: Submit form
SF->>F: Delegate submission & validation
F->>A: Process form submission (validate, update, etc.)
A->>S: Send API request
S-->>A: Return response (success/error)
A-->>F: Wrap response with message()
F->>SF: Update form state via onUpdated callback
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
nas-tabchiche
changed the title
![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 (2)
frontend/src/lib/components/Forms/ModelForm.svelte (2)
118-138: Remove console.log statement.The console.log statement on line 129 should be removed before production deployment.
- console.log('form', form);Optimize cache cleanup in onUpdated callback.
The cache is deleted twice when the form is valid - once at the start of the callback and again after checking form validity.
onUpdated: async ({ form }) => { - createModalCache.deleteCache(model.urlModel); if (form.message?.redirect) { goto(getSecureRedirect(form.message.redirect)); } if (form.valid) { parent.onConfirm(); createModalCache.deleteCache(model.urlModel); } }
141-154: Remove duplicate form configuration props.The
dataTypeandenctypeprops are defined both in the_formconfiguration and directly on theSuperFormcomponent. Since they're already configured in_form, they can be removed from the component props.<SuperForm class="flex flex-col space-y-3" - dataType={shape.attachment ? 'form' : 'json'} - enctype={shape.attachment ? 'multipart/form-data' : 'application/x-www-form-urlencoded'} data={form} {_form} {invalidateAll} let:form let:data let:initialData validators={zod(schema)} onUpdated={() => createModalCache.deleteCache(model.urlModel)} {...$$restProps} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
frontend/src/lib/components/Forms/ModelForm.svelte(3 hunks)frontend/src/lib/utils/actions.ts(8 hunks)frontend/src/routes/(app)/(internal)/[model=urlmodel]/+page.server.ts(2 hunks)frontend/src/routes/(app)/(internal)/[model=urlmodel]/+page.svelte(1 hunks)frontend/src/routes/(app)/(internal)/[model=urlmodel]/[id=uuid]/+page.svelte(0 hunks)frontend/src/routes/(app)/(internal)/ebios-rm/[id=uuid]/+page.svelte(0 hunks)frontend/src/routes/(app)/(third-party)/[model=thirdparty_urlmodels]/+page.svelte(1 hunks)frontend/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/+page.svelte(0 hunks)
💤 Files with no reviewable changes (3)
- frontend/src/routes/(app)/(internal)/[model=urlmodel]/[id=uuid]/+page.svelte
- frontend/src/routes/(app)/(third-party)/compliance-assessments/[id=uuid]/+page.svelte
- frontend/src/routes/(app)/(internal)/ebios-rm/[id=uuid]/+page.svelte
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: migrations-check (3.12)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (8)
frontend/src/routes/(app)/(third-party)/[model=thirdparty_urlmodels]/+page.svelte (1)
5-7: LGTM!The removal of redirection logic aligns with the PR objective to centralize post-validation behaviors in
onUpdatedEvent.frontend/src/routes/(app)/(internal)/[model=urlmodel]/+page.server.ts (1)
11-11: Improved error handling with structured messages.Using
messagefromsveltekit-superformsprovides a more structured way to handle form responses, especially for error cases. This change aligns with the PR objective of centralizing post-validation behaviors.Also applies to: 124-124
frontend/src/routes/(app)/(internal)/[model=urlmodel]/+page.svelte (1)
5-8: LGTM!The removal of redirection logic and reorganization of imports aligns with the PR objective to centralize post-validation behaviors in
onUpdatedEvent.frontend/src/lib/utils/actions.ts (3)
79-79: Improved error handling inhandleErrorResponse.The function now returns structured messages using
messagefromsveltekit-superforms, providing a more consistent way to handle warnings and errors.Also applies to: 83-83, 88-88
114-114: Enhanced form action responses indefaultWriteFormAction.The function now uses structured messages for all response types:
- Invalid form validation
- Redirection after successful creation
- Object creation success
This provides a more consistent interface for handling form responses.Also applies to: 171-171, 173-173
217-217: Consistent error handling indefaultDeleteFormAction.The function now follows the same pattern of using structured messages for all response types, maintaining consistency with other form actions.
Also applies to: 228-228, 233-233, 245-245
frontend/src/lib/components/Forms/ModelForm.svelte (2)
40-40: LGTM!Clean import combining both the function and type from
sveltekit-superforms.
319-321: LGTM!The save button now correctly uses the form's built-in validation mechanism, aligning with the PR's objective to centralize form validation logic.
…EvidenceWriteSerializer
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/core/serializers.py (1)
9-9: Consider replacing star import with explicit imports.Using star imports (
from core.models import *) makes it harder to track dependencies and can lead to naming conflicts. Consider explicitly importing the required models.-from core.models import * +from core.models import ( + AppliedControl, + Evidence, + RequirementAssessment, + # Add other required models +)🧰 Tools
🪛 Ruff (0.8.2)
9-9:
from core.models import *used; unable to detect undefined names(F403)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/core/serializers.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
backend/core/serializers.py
824-824: AppliedControl may be undefined, or defined from star imports
(F405)
827-827: RequirementAssessment may be undefined, or defined from star imports
(F405)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: test (3.12)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: build (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: ruff (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
backend/core/serializers.py (1)
823-828: LGTM! Making fields optional aligns with form validation refactoring.The changes make
applied_controlsandrequirement_assessmentsfields optional by addingrequired=False. This aligns with the PR objective of centralizing form validation logic.🧰 Tools
🪛 Ruff (0.8.2)
824-824:
AppliedControlmay be undefined, or defined from star imports(F405)
827-827:
RequirementAssessmentmay be undefined, or defined from star imports(F405)
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
frontend/src/lib/components/Forms/ModelForm.svelte (1)
140-153: Remove redundantonUpdatedhandler from SuperForm component.The
onUpdatedhandler is now managed in the_formconfiguration, but it's also present in the SuperForm component. This duplication could lead to the cache being deleted multiple times or cause race conditions.<SuperForm class="flex flex-col space-y-3" dataType={shape.attachment ? 'form' : 'json'} enctype={shape.attachment ? 'multipart/form-data' : 'application/x-www-form-urlencoded'} data={form} {_form} {invalidateAll} let:form let:data let:initialData validators={zod(schema)} - onUpdated={() => createModalCache.deleteCache(model.urlModel)} {...$$restProps} >
🧹 Nitpick comments (1)
frontend/src/lib/components/Forms/ModelForm.svelte (1)
127-136: Remove duplicate cache deletion inonUpdatedcallback.The cache is being deleted twice:
- At the start of the callback (line 128)
- After form validation (line 134)
This redundancy should be removed.
onUpdated: async ({ form }) => { - createModalCache.deleteCache(model.urlModel); if (form.message?.redirect) { goto(getSecureRedirect(form.message.redirect)); } if (form.valid) { parent.onConfirm(); createModalCache.deleteCache(model.urlModel); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/lib/components/Forms/ModelForm.svelte(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: build (3.12)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: Analyze (python)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: test (3.12)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
frontend/src/lib/components/Forms/ModelForm.svelte (3)
40-40: LGTM! Import changes align with centralization goal.The addition of
superFormimport fromsveltekit-superformsaligns with the PR's objective of centralizing form validation logic.
118-137: LGTM! Form configuration effectively centralizes form behavior.The
_formconfiguration successfully centralizes form validation and post-submission behavior, including data type handling, validation method, and navigation logic.
317-320: LGTM! Button handling simplified correctly.The save button now correctly relies on the form's built-in validation and submission handling, which aligns with the centralization of form validation logic.
Mohamed-Hacene
deleted the
refactor/move-post-form-validation-logic-to-superform
branch
This aims to centralize all post-valid behaviours, e.g. redirects,
closing modals...
Summary by CodeRabbit