[679-4] feat(form):enhance validation UX and prevent duplicate submissions - #190
Conversation
📝 WalkthroughWalkthroughThe changes refactor the form widget's markup to use standard HTML attributes and a fixed submission endpoint, removing custom data attributes and inline event handlers. The JavaScript for form validation and submission is rewritten for asynchronous handling, improved error feedback, and prevention of multiple submissions. Form input styling is adjusted, and tests are updated to verify the new async submission and button state logic. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser
participant Form JS
participant Server
User->>Browser: Fills form & clicks submit
Browser->>Form JS: Submit event triggered
Form JS->>Form JS: Validate fields
alt Validation fails
Form JS->>Browser: Show field errors, scroll to first invalid
else Validation passes
Form JS->>Form JS: Disable submit button
Form JS->>Server: Send form data (fetch, JSON, CSRF)
Server-->>Form JS: Responds with success or errors
alt Server returns errors
Form JS->>Browser: Show server errors
Form JS->>Form JS: Re-enable submit button
else Server returns success
Form JS->>Browser: Reset form, hide form, show thank-you
end
end
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
🔍 Vulnerabilities of
|
| digest | sha256:44c13fcdc47481729ec96e30f387705ccad9f84ecb955f66ab311057f6598023 |
| vulnerabilities |     |
| platform | linux/amd64 |
| size | 290 MB |
| packages | 977 |
📦 Base Image node:23-alpine
| also known as |
|
| digest | sha256:b9d38d589853406ff0d4364f21969840c3e0397087643aef8eede40edbb6c7cd |
| vulnerabilities |   |
Description
| ||||||||||||
Description
| ||||||||||||
Description
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
website/modules/asset/ui/src/js/formValidation.test.js (1)
3-9: Fix formatting and improve code structure.The fetch mock setup has formatting issues:
-if (!global.fetch) { - global.fetch = jest.fn(() => - Promise.resolve({ - json: () => Promise.resolve({ success: true }), - }), - ); -} +if (!global.fetch) { + global.fetch = jest.fn(() => + Promise.resolve({ + json: () => Promise.resolve({ success: true }), + }), + ); +}website/modules/asset/ui/src/js/formValidation.js (4)
63-65: Break long selector into multiple lines for readability.The querySelector string exceeds the 80-character line limit.
- const fields = form.querySelectorAll( - 'input:not([type="submit"]):not([type="button"]):not([type="hidden"]), textarea, select', - ); + const fields = form.querySelectorAll( + 'input:not([type="submit"]):not([type="button"]):not([type="hidden"]), ' + + 'textarea, select', + );
99-99: Use arrow function for simple one-liner.The function can be simplified to match the coding guidelines for concise functions.
-const collectFormData = (form) => new FormData(form); +const collectFormData = (form) => new FormData(form);Actually, this is already correct. No change needed.
185-209: Consider extracting complex logic into smaller functions.The
sendFormDatafunction has multiple responsibilities: data transformation and fetch request. Consider breaking it down for better maintainability.+const transformFormDataToObject = (formData) => { + const data = {}; + for (const [key, value] of formData.entries()) { + if (key in data) { + data[key] = [].concat(data[key], value); + } else { + data[key] = value; + } + } + return data; +}; + const sendFormData = (form, formData) => { - // Convert FormData to a plain object for the server - const data = {}; - for (const [key, value] of formData.entries()) { - if (key in data) { - data[key] = [].concat(data[key], value); - } else { - data[key] = value; - } - } + const data = transformFormDataToObject(formData); return fetch(form.action, { method: 'POST', body: JSON.stringify({ data }), headers: { 'Content-Type': 'application/json', 'Accept': 'application/json', 'CSRF-Token': document .querySelector('meta[name="csrf-token"]') ?.getAttribute('content') || '', }, credentials: 'same-origin', }); };
228-243: Break long selector into multiple lines for consistency.Similar to the earlier selector, this one also exceeds line length limits.
const fields = form.querySelectorAll( - 'input:not([type="submit"]):not([type="button"]):not([type="hidden"]), textarea, select', + 'input:not([type="submit"]):not([type="button"]):not([type="hidden"]), ' + + 'textarea, select', );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
website/modules/@apostrophecms/form-widget/index.js(1 hunks)website/modules/@apostrophecms/form-widget/views/widget.html(3 hunks)website/modules/asset/ui/src/js/formValidation.js(2 hunks)website/modules/asset/ui/src/js/formValidation.test.js(2 hunks)website/modules/asset/ui/src/scss/_form.scss(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
`**/*.{js,jsx}`: Use 2 spaces for indentation Maximum line length: 80 characters...
**/*.{js,jsx}: Use 2 spaces for indentation
Maximum line length: 80 characters
Use semicolons at the end of statements
Use single quotes for strings
Add trailing commas in arrays, objects, etc.
No trailing spaces
Use consistent quote properties (either quote all properties or none)
Place line comments above code, not inline
Capitalize all comments
No inline comments
Maximum function length enforced (avoid excessive length)
Maximum depth: 4 levels
Maximum callback nesting: 3 levels
Maximum parameters: 5
Maximum statements per function: 50
Use function declarations with named functions, not function expressions
Always initialize variables at declaration
Maximum lines per file: 300
Use destructuring where possible
Minimum identifier length enforced (no single-letter variables)
No unused variables
No reassignment of function parameters
No invalid 'this' context
No duplicate object keys
No ternary operators (use if/else)
Maximum complexity: 15 (cognitive complexity)
No alerts or console logs
No debugger statements
No identical expressions in conditions
Use optimized regex patterns
Use Unicode regex patterns
No secrets in code
No unsanitized methods or properties (XSS prevention)
Sort imports alphabetically
No unresolved imports
No importing default from a module that doesn't have a default export
Always return in promise chains
📄 Source: CodeRabbit Inference Engine (.cursor/rules/code-conventions.mdc)
List of files the instruction was applied to:
website/modules/@apostrophecms/form-widget/index.jswebsite/modules/asset/ui/src/js/formValidation.test.jswebsite/modules/asset/ui/src/js/formValidation.js
`**/*.js`: No missing imports in Node.js No missing require statements
**/*.js: No missing imports in Node.js
No missing require statements
📄 Source: CodeRabbit Inference Engine (.cursor/rules/code-conventions.mdc)
List of files the instruction was applied to:
website/modules/@apostrophecms/form-widget/index.jswebsite/modules/asset/ui/src/js/formValidation.test.jswebsite/modules/asset/ui/src/js/formValidation.js
`**/*.test.{js,jsx}`: Test files have relaxed rules for function length, statements, extraneous requires, and destructuring
**/*.test.{js,jsx}: Test files have relaxed rules for function length, statements, extraneous requires, and destructuring
📄 Source: CodeRabbit Inference Engine (.cursor/rules/code-conventions.mdc)
List of files the instruction was applied to:
website/modules/asset/ui/src/js/formValidation.test.js
🧠 Learnings (6)
📓 Common learnings
Learnt from: yuramax
PR: speedandfunction/website#120
File: website/modules/asset/ui/src/index.js:43-67
Timestamp: 2025-05-26T06:19:32.209Z
Learning: When reviewing PRs focused on styling, defer functional JavaScript improvements to separate tasks to maintain clear scope separation.
Learnt from: VitalyyP
PR: speedandfunction/website#169
File: website/modules/asset/ui/src/js/formValidation.js:68-71
Timestamp: 2025-06-10T14:41:04.740Z
Learning: In the website project, form validation is intentionally scoped to forms with the `.sf-form` class, which is the class used in their form widget template. This ensures validation only applies to forms created by their specific widget, not all forms on the page.
Learnt from: VitalyyP
PR: speedandfunction/website#169
File: website/modules/asset/ui/src/js/formValidation.js:68-71
Timestamp: 2025-06-10T14:41:10.045Z
Learning: In the website project, the `.sf-form` CSS class selector is intentionally used in form validation because it's the class used in the form widget template, not an inconsistency that needs to be fixed.
website/modules/@apostrophecms/form-widget/index.js (10)
Learnt from: CR
PR: speedandfunction/website#0
File: .cursor/rules/code-conventions.mdc:0-0
Timestamp: 2025-07-01T07:44:33.034Z
Learning: Applies to modules/asset/ui/src/index.js : modules/asset/ui/src/index.js: relaxed function lines and style rules
Learnt from: CR
PR: speedandfunction/website#0
File: .cursor/rules/code-conventions.mdc:0-0
Timestamp: 2025-07-01T07:44:33.034Z
Learning: Applies to modules/@apostrophecms/form/index.js : modules/@apostrophecms/form/index.js: relaxed maximum lines rule
Learnt from: CR
PR: speedandfunction/website#0
File: .cursor/rules/code-conventions.mdc:0-0
Timestamp: 2025-07-01T07:44:33.034Z
Learning: Applies to modules/asset/ui/src/swipers.js : modules/asset/ui/src/swipers.js: relaxed import rules
Learnt from: IhorMasechko
PR: speedandfunction/website#154
File: website/modules/case-studies-page/services/UrlService.js:102-116
Timestamp: 2025-06-09T16:52:54.510Z
Learning: When reviewing ApostropheCMS code, eslint-disable comments for `no-underscore-dangle` are often necessary and appropriate when accessing framework-provided properties like `_url`, as they conflict with general JavaScript linting rules but follow ApostropheCMS conventions.
Learnt from: CR
PR: speedandfunction/website#0
File: .cursor/rules/code-conventions.mdc:0-0
Timestamp: 2025-07-01T07:44:33.034Z
Learning: Applies to **/*.{js,jsx} : No unresolved imports
Learnt from: CR
PR: speedandfunction/website#0
File: .cursor/rules/code-conventions.mdc:0-0
Timestamp: 2025-07-01T07:44:33.034Z
Learning: Applies to **/*.{js,jsx} : No importing default from a module that doesn't have a default export
Learnt from: CR
PR: speedandfunction/website#0
File: .cursor/rules/code-conventions.mdc:0-0
Timestamp: 2025-07-01T07:44:33.034Z
Learning: Applies to **/*.{js,jsx} : Use destructuring where possible
Learnt from: CR
PR: speedandfunction/website#0
File: .cursor/rules/code-conventions.mdc:0-0
Timestamp: 2025-07-01T07:44:33.034Z
Learning: Applies to **/*.js : No missing require statements
Learnt from: CR
PR: speedandfunction/website#0
File: .cursor/rules/code-conventions.mdc:0-0
Timestamp: 2025-07-01T07:44:33.034Z
Learning: Applies to **/*.{js,jsx} : No duplicate object keys
Learnt from: CR
PR: speedandfunction/website#0
File: .cursor/rules/code-conventions.mdc:0-0
Timestamp: 2025-07-01T07:44:33.034Z
Learning: Applies to **/*.js : No missing imports in Node.js
website/modules/asset/ui/src/scss/_form.scss (6)
Learnt from: yuramax
PR: speedandfunction/website#121
File: website/modules/asset/ui/src/scss/_vacancies.scss:99-102
Timestamp: 2025-05-26T15:57:01.766Z
Learning: In website/modules/asset/ui/src/scss/_vacancies.scss, the margin shorthand (e.g., `margin: 0 0 8px`) in `.sf-vacancies_footer h2` is intentionally used to reset all margins and set specific bottom margins, rather than using `margin-bottom` alone. This approach ensures clean baseline styling that doesn't inherit unwanted margins, while still allowing the sibling selector `.vacancy-card + .sf-vacancies_footer h2` to add top margins when a vacancy card precedes the footer.
Learnt from: IhorMasechko
PR: speedandfunction/website#132
File: website/modules/asset/ui/src/scss/_not-found.scss:52-64
Timestamp: 2025-05-29T07:16:52.843Z
Learning: In website/modules/asset/ui/src/scss/_not-found.scss, the .two-buttons container with flex-direction: row and child .sf-button elements having width: 100% does not cause overflow issues and renders correctly, despite theoretical expectations.
Learnt from: VitalyyP
PR: speedandfunction/website#169
File: website/modules/asset/ui/src/js/formValidation.js:68-71
Timestamp: 2025-06-10T14:41:10.045Z
Learning: In the website project, the `.sf-form` CSS class selector is intentionally used in form validation because it's the class used in the form widget template, not an inconsistency that needs to be fixed.
Learnt from: CR
PR: speedandfunction/website#0
File: .cursor/rules/code-conventions.mdc:0-0
Timestamp: 2025-07-01T07:44:33.034Z
Learning: Applies to modules/asset/ui/src/index.js : modules/asset/ui/src/index.js: relaxed function lines and style rules
Learnt from: CR
PR: speedandfunction/website#0
File: .cursor/rules/code-conventions.mdc:0-0
Timestamp: 2025-07-01T07:44:33.034Z
Learning: Applies to modules/@apostrophecms/form/index.js : modules/@apostrophecms/form/index.js: relaxed maximum lines rule
Learnt from: VitalyyP
PR: speedandfunction/website#169
File: website/modules/asset/ui/src/js/formValidation.js:68-71
Timestamp: 2025-06-10T14:41:04.740Z
Learning: In the website project, form validation is intentionally scoped to forms with the `.sf-form` class, which is the class used in their form widget template. This ensures validation only applies to forms created by their specific widget, not all forms on the page.
website/modules/asset/ui/src/js/formValidation.test.js (1)
Learnt from: CR
PR: speedandfunction/website#0
File: .cursor/rules/code-conventions.mdc:0-0
Timestamp: 2025-07-01T07:44:33.034Z
Learning: Applies to modules/@apostrophecms/form/index.js : modules/@apostrophecms/form/index.js: relaxed maximum lines rule
website/modules/@apostrophecms/form-widget/views/widget.html (4)
Learnt from: CR
PR: speedandfunction/website#0
File: .cursor/rules/code-conventions.mdc:0-0
Timestamp: 2025-07-01T07:44:33.034Z
Learning: Applies to modules/@apostrophecms/form/index.js : modules/@apostrophecms/form/index.js: relaxed maximum lines rule
Learnt from: VitalyyP
PR: speedandfunction/website#155
File: website/modules/@apostrophecms/form/index.js:7-18
Timestamp: 2025-06-06T07:47:18.719Z
Learning: In website/modules/@apostrophecms/form/index.js, the parseFormData function intentionally does not include try-catch for JSON.parse errors to avoid nested error handling. Errors are allowed to bubble up to higher-level handlers where they can be properly logged and handled, keeping the error handling architecture simpler and more maintainable.
Learnt from: VitalyyP
PR: speedandfunction/website#169
File: website/modules/asset/ui/src/js/formValidation.js:68-71
Timestamp: 2025-06-10T14:41:04.740Z
Learning: In the website project, form validation is intentionally scoped to forms with the `.sf-form` class, which is the class used in their form widget template. This ensures validation only applies to forms created by their specific widget, not all forms on the page.
Learnt from: VitalyyP
PR: speedandfunction/website#169
File: website/modules/asset/ui/src/js/formValidation.js:68-71
Timestamp: 2025-06-10T14:41:10.045Z
Learning: In the website project, the `.sf-form` CSS class selector is intentionally used in form validation because it's the class used in the form widget template, not an inconsistency that needs to be fixed.
website/modules/asset/ui/src/js/formValidation.js (4)
Learnt from: VitalyyP
PR: speedandfunction/website#169
File: website/modules/asset/ui/src/js/formValidation.js:68-71
Timestamp: 2025-06-10T14:41:04.740Z
Learning: In the website project, form validation is intentionally scoped to forms with the `.sf-form` class, which is the class used in their form widget template. This ensures validation only applies to forms created by their specific widget, not all forms on the page.
Learnt from: VitalyyP
PR: speedandfunction/website#155
File: website/modules/@apostrophecms/form/index.js:7-18
Timestamp: 2025-06-06T07:47:18.719Z
Learning: In website/modules/@apostrophecms/form/index.js, the parseFormData function intentionally does not include try-catch for JSON.parse errors to avoid nested error handling. Errors are allowed to bubble up to higher-level handlers where they can be properly logged and handled, keeping the error handling architecture simpler and more maintainable.
Learnt from: CR
PR: speedandfunction/website#0
File: .cursor/rules/code-conventions.mdc:0-0
Timestamp: 2025-07-01T07:44:33.034Z
Learning: Applies to modules/@apostrophecms/form/index.js : modules/@apostrophecms/form/index.js: relaxed maximum lines rule
Learnt from: CR
PR: speedandfunction/website#0
File: .cursor/rules/code-conventions.mdc:0-0
Timestamp: 2025-07-01T07:44:33.034Z
Learning: Applies to modules/asset/ui/src/index.js : modules/asset/ui/src/index.js: relaxed function lines and style rules
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: lint
- GitHub Check: e2e-tests
- GitHub Check: unit-tests
- GitHub Check: security-scan
🔇 Additional comments (4)
website/modules/asset/ui/src/scss/_form.scss (1)
43-49: LGTM - Clean separation of input height styling.The extraction of height styling for
.sf-inputinto a separate selector improves maintainability by allowing independent height control for inputs versus textareas.website/modules/@apostrophecms/form-widget/views/widget.html (2)
19-21: LGTM - Form attributes updated for modern async handling.The addition of
novalidate,method="post", and fixedactionURL properly supports the new asynchronous form validation and submission flow.
80-80: LGTM - Quote style consistency.Using single quotes aligns with the JavaScript coding guidelines.
website/modules/asset/ui/src/js/formValidation.js (1)
211-226: LGTM - Excellent duplicate submission prevention.The form submission handler properly disables submit buttons during processing and re-enables them afterward, effectively preventing duplicate submissions while maintaining good UX.
VitalyyP
changed the title
|
5 participants
Uh oh!
There was an error while loading. Please reload this page.