Fix form client validation - #175
Conversation
📝 WalkthroughWalkthroughThis change introduces a new POST route for form submission, refactors client-side form validation to support asynchronous submission, and updates form templates to use static attributes and remove inline event handlers. Checkbox field handling is enhanced in both validation and template rendering, and form-related assets and styles are reorganized for clarity and modularity. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser
participant ClientJS
participant Server
participant FormModule
User->>Browser: Submits form
Browser->>ClientJS: Triggers async submit handler
ClientJS->>ClientJS: Validate fields (including checkboxes)
alt Validation fails
ClientJS->>Browser: Scroll/focus first invalid field, show error
else Validation passes
ClientJS->>Server: POST /api/v1/@apostrophecms/form/submit (JSON)
Server->>FormModule: Handle POST, parse and validate data
alt Data invalid
FormModule->>Server: Respond 400 error
Server->>ClientJS: Return error response
ClientJS->>Browser: Show error message
else Submission succeeds
FormModule->>Server: Respond with success JSON
Server->>ClientJS: Return success response
ClientJS->>Browser: Reset form, show thank you message
end
end
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
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:a723c34a49605ea0f39241cbadef4ebb7968781b0452cab84428caa0a5765ecf |
| vulnerabilities |     |
| platform | linux/amd64 |
| size | 290 MB |
| packages | 976 |
📦 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: 6
🔭 Outside diff range comments (2)
website/scripts/generate_constants.js.rej (1)
1-21:⚠️ Potential issueRemove stray patch-reject file from the repository
*.rejfiles are created by a failedgit apply / patchand should never be committed.
Please delete the file and add it to.gitignore(see prior comment).-website/scripts/generate_constants.js.rejwebsite/modules/@apostrophecms/form/index.js (1)
83-90: 🛠️ Refactor suggestionDuplicate submission handling – risk of divergence
self.submitFormnow callsthis.handleFormSubmission, while the new API route talks toself.formSubmissionHandlerdirectly. Any future change to validation/side-effects added tohandleFormSubmissionwill not run for API calls. Consider delegating both call-sites to a single shared helper.
🧹 Nitpick comments (6)
website/.gitignore (1)
18-20: Add pattern for patch-reject artefactsA
.rejfile slipped into the PR (seescripts/generate_constants.js.rej).
Adding a catch-all ignore will prevent this in future.# Generated files modules/@apostrophecms/shared-constants/ui/src/index.js +*.rejwebsite/scripts/generate_constants.js (1)
47-51: String-interpolation can break on single quotes / edge casesBuilding the object literal via template concatenation risks producing invalid JS if any value contains a single quote or backslash. A safer (and slightly shorter) approach is:
-export const STANDARD_FORM_FIELD_NAMES = { - ${Object.entries(STANDARD_FORM_FIELD_NAMES) - .map(([key, value]) => `${key}: '${value}'`) - .join(',\n ')}, -}; +export const STANDARD_FORM_FIELD_NAMES = ${JSON.stringify( + STANDARD_FORM_FIELD_NAMES, + null, + 2 +)};The output is still a readable object literal and requires no manual escaping.
website/modules/asset/ui/src/js/formValidation.test.js (1)
3-10: Mockfetchmore completely
Down-stream code often checksresponse.okorresponse.status. The current stub only exposesjson(). A simple addition avoids fragile tests:global.fetch = jest.fn(() => Promise.resolve({ + ok: true, + status: 200, json: () => Promise.resolve({ success: true }), }), );website/modules/@apostrophecms/form-widget/views/widget.html (1)
79-80: Inlinealertviolates strict CSP
Hard-coded inline JS may be blocked if a Content-Security-Policy withscript-src 'self'is enforced. Consider moving this callback to the bundled client script and reference it via a data attribute.website/modules/asset/ui/src/js/formValidation.js (2)
121-129: Tighten optional-chaining & readability
error && error.textContentcan be collapsed:-if (error && error.textContent && error.textContent.trim() !== '') { +if (error?.textContent?.trim()) {Minor, but shortens the guard and matches the optional-chain pattern flagged by the linter.
🧰 Tools
🪛 Biome (1.9.4)
[error] 125-125: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
154-162:thankYouselector is global – restrict to current form container
document.querySelector('[data-apos-form-thank-you]')will only work for a single form. Scope the search toform.closest('[data-form-wrapper]')or similar to avoid clobbering parallel forms.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
eslint-form-fields-solution.patch(1 hunks)website/.gitignore(1 hunks)website/app.js(1 hunks)website/modules/@apostrophecms/form-widget/index.js(1 hunks)website/modules/@apostrophecms/form-widget/views/widget.html(3 hunks)website/modules/@apostrophecms/form/index.js(1 hunks)website/modules/@apostrophecms/shared-constants/ui/src/index.js(0 hunks)website/modules/asset/ui/index.js(1 hunks)website/modules/asset/ui/src/js/formValidation.js(1 hunks)website/modules/asset/ui/src/js/formValidation.test.js(1 hunks)website/scripts/generate_constants.js(1 hunks)website/scripts/generate_constants.js.rej(1 hunks)
💤 Files with no reviewable changes (1)
- website/modules/@apostrophecms/shared-constants/ui/src/index.js
🧰 Additional context used
🧬 Code Graph Analysis (1)
website/scripts/generate_constants.js (1)
website/modules/@apostrophecms/shared-constants/index.js (1)
STANDARD_FORM_FIELD_NAMES(6-10)
🪛 Biome (1.9.4)
website/modules/asset/ui/src/js/formValidation.js
[error] 125-125: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: e2e-tests
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: security-scan
🔇 Additional comments (9)
website/.gitignore (1)
18-20: Good call on ignoring the generated constants fileKeeping generated artefacts out of version control avoids noisy diffs and merge pain.
website/scripts/generate_constants.js (1)
56-57: Trailing comma after last property is fine but check lint rulesES2017 allows trailing commas in object literals; however some code-style configs (e.g.
airbnb-base) forbid them. Confirm your linter agrees; otherwise CI will fail.website/modules/asset/ui/index.js (1)
1-1: Entry point looks goodSide-effect import cleanly re-exports the UI bundle; no issues spotted.
website/app.js (1)
41-41: Module key updated correctlyRenaming to
'@apostrophecms/shared-constants'keeps config consistent with filesystem layout. 👍website/modules/@apostrophecms/form-widget/index.js (1)
17-19: Confirm asset path/mathcing build alias
'module:asset/ui/src/index.js'assumes an alias calledassetis registered with Apostrophe’s asset bundler. If that alias is missing or renamed (the physical path iswebsite/modules/asset/ui/src/index.js), the script will never be shipped to the browser and client-side validation will silently break.Please double-check the
modules/assetbundle configuration (usually in the rootapp.js) or runapos dev:inspectto be sure the alias resolves.website/modules/asset/ui/src/js/formValidation.test.js (1)
20-25:SubmitEventis not available in JSDOM
JSDOM shipped with Jest (v28-29) still lacks a nativeSubmitEventconstructor. This line may throw on CI without a polyfill.Verify test run on your CI node version. If it fails, fall back to
new Event('submit', { cancelable: true })or install the polyfill.website/modules/@apostrophecms/form/index.js (1)
104-108: 🛠️ Refactor suggestion
JSON.parseerror not handled insideparseFormData
IfrawDatais a malformed JSON stringJSON.parsewill throw and bubble up to the outercatch, returning HTTP 500 instead of a clean 400.if (typeof rawData === 'string') { - return JSON.parse(rawData); + try { + return JSON.parse(rawData); + } catch { + return null; + } }⛔ Skipped due to learnings
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.website/modules/@apostrophecms/form-widget/views/widget.html (1)
34-35: Button always disabled when reCAPTCHA enabled
The button is rendereddisabledbut nodata-*hook remains to enable it after reCAPTCHA resolves, so the form becomes unusable with JS off or if the enabling code expects the removed attribute. Verify the enabling script matches this markup.website/modules/asset/ui/src/js/formValidation.js (1)
179-188: Consider addingcredentials: 'same-origin'to include cookiesIf the endpoint relies on session cookies/CSRF tokens, omit this and the browser may drop them on CORS-like requests.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
website/modules/asset/ui/src/js/formValidation.js (3)
99-115: 🛠️ Refactor suggestionStill manually iterating the form – please switch to
FormDataAPIWe previously suggested replacing this bespoke loop with the native
FormDataconstructor to correctly capture multi-value fields, checkboxes, files, etc. The current code still misses those edge-cases and is harder to maintain.-const collectFormData = (form) => { - const formElements = form.elements; - const formData = {}; - let index = 0; - while (index < formElements.length) { - const element = formElements[index]; - if ( - element.name && - element.type !== 'submit' && - element.type !== 'button' - ) { - formData[element.name] = element.value; - } - index += 1; - } - return formData; -}; +const collectFormData = (form) => { + const data = {}; + const fd = new FormData(form); + for (const [key, value] of fd.entries()) { + data[key] = key in data ? [].concat(data[key], value) : value; + } + return data; +};
159-175:⚠️ Potential issueFetch response is parsed without
response.okguard – JSON parse will throw on 4xx/5xx
handleServerResponsealways callsresponse.json(); if the server returns a non-OK status or empty body the promise rejects and we silently swallow it in.catch(() => null). Wrap the parse in a status check and propagate the error so the UI can react.-const handleServerResponse = (response, form) => { - return response - .json() - .then((data) => onHandleServerResponse(data, form)) - .catch(() => null); -}; +const handleServerResponse = async (response, form) => { + if (!response.ok) { + throw new Error(`Request failed: ${response.status}`); + } + const data = await response.json(); + return onHandleServerResponse(data, form); +};
147-157: 🛠️ Refactor suggestionErrors are still silenced – user receives no feedback
Every network / parse failure ultimately resolves to
null, echoing the earlier concern about “silent no-ops”. At minimum log the error; ideally surface a generic banner so the visitor is not left wondering.
🧹 Nitpick comments (1)
website/modules/asset/ui/src/js/formValidation.js (1)
150-155:document.querySelectorlimits multi-form supportUsing the global document selector will only reveal the first
[data-apos-form-thank-you]element. If multiple forms exist, show the thank-you message scoped to the current form (or its container) to avoid interference.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.gitignore(1 hunks)website/modules/asset/ui/src/js/formValidation.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🧰 Additional context used
🪛 Biome (1.9.4)
website/modules/asset/ui/src/js/formValidation.js
[error] 124-124: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: e2e-tests
- GitHub Check: security-scan
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (actions)
🔇 Additional comments (1)
website/modules/asset/ui/src/js/formValidation.js (1)
121-124: Optional-chaining can simplify the null guards-const error = field - .closest('.sf-field') - ?.querySelector('.validation-error'); +const error = field.closest('.sf-field')?.querySelector('.validation-error');[nitpick]
🧰 Tools
🪛 Biome (1.9.4)
[error] 124-124: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
website/modules/asset/ui/src/js/formValidation.js (3)
99-115: 🛠️ Refactor suggestionSwitch to the native
FormDataAPI – manual looping still misses edge-cases
This was raised earlier and remains unresolved.The current loop drops:
• multiple inputs with the same name
• checked/unchecked state of checkboxes & radios
•<select multiple>values
•<input type="file">objects-const collectFormData = (form) => { - const formElements = form.elements; - const formData = {}; - let index = 0; - while (index < formElements.length) { - const element = formElements[index]; - if ( - element.name && - element.type !== 'submit' && - element.type !== 'button' - ) { - formData[element.name] = element.value; - } - index += 1; - } - return formData; -}; +const collectFormData = (form) => { + const data = {}; + const fd = new FormData(form); + for (const [key, value] of fd.entries()) { + data[key] = key in data ? [].concat(data[key], value) : value; + } + return data; +};
160-165:⚠️ Potential issue
response.okisn’t checked – JSON parsing will throw on 4xx/5xxSame concern as the previous review: call
response.json()only for successful responses and route errors toonSendFormDataResponse/UI notification.-return response - .json() - .then((data) => onHandleServerResponse(data, form)) - .catch(() => null); +if (!response.ok) { + return Promise.reject(new Error(`Request failed: ${response.status}`)); +} +return response + .json() + .then((data) => onHandleServerResponse(data, form)) + .catch(() => null);
148-156: 🛠️ Refactor suggestionUser never sees server-side errors – surface them instead of silent
null
onHandleServerResponsereturnsnullfor any non-success path, leaving the user without feedback. Log or display a generic banner so the failure is visible.Minimum:
-} -return null; +} +showValidationErrorFn( + form, + 'Submission failed. Please try again later.', +);
🧹 Nitpick comments (4)
website/modules/csrf-helper/index.js (1)
8-21: Duplicate helper & method → consider DRY
getCsrfTokenis declared twice with identical logic (inextendMethodsandextendHelpers).
Exporting it only once and re-using would reduce duplication.website/modules/asset/ui/src/js/domHelpers.js (1)
5-9: RepeatederrorClassbranching – pull up to a helper
showValidationErrorandclearValidationErrorboth recomputeerrorClass. Extracting to a tiny helper would avoid divergence.- let errorClass = 'validation-error'; - if (field.type === 'checkbox') { - errorClass = 'apos-form-error'; - } + const errorClass = field.type === 'checkbox' + ? 'apos-form-error' + : 'validation-error';website/modules/@apostrophecms/form-checkboxes-field-widget/views/widget.html (2)
1-16: Template control flow injects bare{%tokens into markupSplitting the
data-requiredattribute across{% if %}/{% endif %}leaves raw templating tokens inside a multiline attribute list which some HTML linters (HTMLHint) flag as duplicate/invalid attributes.
A compact one-liner is clearer and avoids the warning:<div data-apos-form-checkboxes class="apos-form-input-wrapper sf-field" {{ widget.required ? 'data-required="true"' : '' }}>🧰 Tools
🪛 HTMLHint (1.5.0)
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
[error] 13-13: Duplicate of attribute name [ {% ] was found.
(attr-no-duplication)
17-35:fieldsetlacksaria-requiredfor accessibilityScreen readers rely on ARIA when native
requiredcannot be used (checkbox groups).
Consider:<fieldset class="apos-form-fieldset" {{ widget.required ? 'aria-required="true"' : '' }}>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
website/app.js(2 hunks)website/modules/@apostrophecms/form-checkboxes-field-widget/views/widget.html(1 hunks)website/modules/@apostrophecms/form/index.js(2 hunks)website/modules/asset/ui/src/js/domHelpers.js(2 hunks)website/modules/asset/ui/src/js/formValidation.js(2 hunks)website/modules/asset/ui/src/js/formValidator.js(1 hunks)website/modules/asset/ui/src/js/formValidator.test.js(1 hunks)website/modules/asset/ui/src/js/validationSchemas.js(1 hunks)website/modules/asset/ui/src/scss/_form.scss(1 hunks)website/modules/csrf-helper/index.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- website/modules/asset/ui/src/scss/_form.scss
🚧 Files skipped from review as they are similar to previous changes (2)
- website/app.js
- website/modules/@apostrophecms/form/index.js
🧰 Additional context used
🧬 Code Graph Analysis (1)
website/modules/asset/ui/src/js/domHelpers.js (3)
website/modules/asset/ui/src/js/formValidation.js (5)
field(120-120)form(6-6)form(14-14)errorMessage(7-7)errorMessage(15-15)website/modules/asset/ui/src/js/domHelpers.test.js (2)
field(4-4)wrapper(5-5)website/modules/asset/ui/src/js/formValidation.test.js (2)
form(13-13)errorMessage(15-15)
🪛 HTMLHint (1.5.0)
website/modules/@apostrophecms/form-checkboxes-field-widget/views/widget.html
[error] 1-1: Doctype must be declared before any non-comment content.
(doctype-first)
[error] 13-13: Duplicate of attribute name [ {% ] was found.
(attr-no-duplication)
🪛 Biome (1.9.4)
website/modules/asset/ui/src/js/formValidation.js
[error] 124-124: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: e2e-tests
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: security-scan
🔇 Additional comments (2)
website/modules/asset/ui/src/js/formValidator.test.js (1)
46-50: 👍 AddedhasAttributemock completes the spy surfaceThe validator now checks
element.hasAttribute('required'); adding the stub keeps the unit tests hermetic.
Looks good.website/modules/asset/ui/src/js/validationSchemas.js (1)
83-106: Checkbox schema assumes scalar boolean – verify multi-checkbox use-caseThe fallback schema treats a checkbox field as a single
boolean, which is correct for a lone checkbox but not for a group (the new widget renders many checkboxes with the samename).
If the validator receives the value of one input, this is fine. If it aggregates all checked values into an array, the schema will fail.Please confirm the runtime shape before merge.
- Add CSRF token handling in form submissions - Improve form validation safety checks - Add null checks for DOM operations - Update session configuration - Fix checkbox value handling in form validator - Clean up commented code and improve error handling
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
website/modules/asset/ui/src/js/formValidation.js (2)
159-164: Still parsing JSON without checkingresponse.okCalling
response.json()on a 4xx/5xx or empty body will throw. Verify the status first and route to error handling.- return response - .json() - .then((data) => onHandleServerResponse(data, form)) - .catch(() => null); + if (!response.ok) { + return Promise.reject( + new Error(`Request failed with status ${response.status}`), + ); + } + return response + .json() + .then((data) => onHandleServerResponse(data, form)) + .catch(() => null);
99-115: 🛠️ Refactor suggestionLeverage
FormDatainstead of manual iterationThe custom loop misses edge-cases (multiple values per key, checked-only checkboxes, files, etc.) and duplicates native functionality. Replacing it with the platform API simplifies the code and improves correctness.
-const collectFormData = (form) => { - const formElements = form.elements; - const formData = {}; - let index = 0; - while (index < formElements.length) { - const element = formElements[index]; - if ( - element.name && - element.type !== 'submit' && - element.type !== 'button' - ) { - formData[element.name] = element.value; - } - index += 1; - } - return formData; -}; +const collectFormData = (form) => { + const data = {}; + for (const [key, value] of new FormData(form).entries()) { + data[key] = key in data ? [].concat(data[key], value) : value; + } + return data; +};
🧹 Nitpick comments (1)
website/modules/asset/ui/src/js/formValidation.js (1)
117-126: Condense null/undefined guards with optional chainingMinor readability tweak – the three property checks can be collapsed.
- const error = field - .closest('.sf-field') - ?.querySelector('.validation-error'); - if (error && error.textContent && error.textContent.trim() !== '') { + const hasError = + field + .closest('.sf-field') + ?.querySelector('.validation-error') + ?.textContent?.trim(); + if (hasError) {🧰 Tools
🪛 Biome (1.9.4)
[error] 124-124: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
website/app.js(2 hunks)website/modules/@apostrophecms/form/index.js(2 hunks)website/modules/asset/ui/src/js/domHelpers.js(2 hunks)website/modules/asset/ui/src/js/formValidation.js(2 hunks)website/modules/asset/ui/src/js/formValidation.test.js(1 hunks)website/modules/asset/ui/src/js/formValidator.js(1 hunks)website/modules/csrf-helper/index.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- website/modules/asset/ui/src/js/formValidation.test.js
- website/app.js
- website/modules/asset/ui/src/js/domHelpers.js
- website/modules/@apostrophecms/form/index.js
- website/modules/csrf-helper/index.js
- website/modules/asset/ui/src/js/formValidator.js
🧰 Additional context used
🪛 Biome (1.9.4)
website/modules/asset/ui/src/js/formValidation.js
[error] 124-124: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: e2e-tests
- GitHub Check: security-scan
…guration to main app config - Add better error handling in form validation - Remove redundant CSRF helper module - Improve return values in form validation functions
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
website/modules/asset/ui/src/js/formValidation.js (3)
99-115: Leverage the built-inFormDataAPI instead of a manual loopManual traversal misses edge-cases (checkbox groups, multi-selects, files, multiple inputs with the same name) and duplicates logic that the platform already provides.
-const collectFormData = (form) => { - const formElements = form.elements; - const formData = {}; - let index = 0; - while (index < formElements.length) { - const element = formElements[index]; - if ( - element.name && - element.type !== 'submit' && - element.type !== 'button' - ) { - formData[element.name] = element.value; - } - index += 1; - } - return formData; -}; +const collectFormData = (form) => { + const data = {}; + const fd = new FormData(form); + for (const [key, value] of fd.entries()) { + data[key] = key in data ? [].concat(data[key], value) : value; + } + return data; +};This single change correctly handles all field types and simplifies the code.
167-172: Checkresponse.okbefore parsing JSON
response.json()will throw on 4xx/5xx or empty bodies.- return response - .json() - .then((data) => onHandleServerResponse(data, form)) - .catch(() => false); + if (!response.ok) { + return false; + } + return response + .json() + .then((data) => onHandleServerResponse(data, form)) + .catch(() => false);Avoids unhandled promise rejections and keeps error handling consistent.
140-147: 🛠️ Refactor suggestionSurface network/validation errors instead of swallowing them
The
catchblock hides the underlying error; logging it greatly eases debugging.- .catch(() => { + .catch((err) => { + /* eslint-disable no-console */ + console.error('Form submission failed', err); const errorMessage = form.querySelector('.error-message'); if (errorMessage) { errorMessage.textContent = 'Failed to submit form. Please try again later.'; } return null; });Provides visibility without altering behaviour for end-users.
🧹 Nitpick comments (3)
website/modules/asset/ui/src/js/formValidation.js (3)
118-129: Minor: use optional chaining to simplify null-checksLines 124-126 can be condensed and made safer:
- const error = field - .closest('.sf-field') - ?.querySelector('.validation-error'); - if (error && error.textContent && error.textContent.trim() !== '') { + const error = + field + .closest('.sf-field') + ?.querySelector('.validation-error')?.textContent?.trim(); + if (error) { field.scrollIntoView({ behavior: 'smooth', block: 'center' }); field.focus(); break; }Less branching, clearer intent.
🧰 Tools
🪛 Biome (1.9.4)
[error] 124-124: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
154-162: Scope the “thank-you” search to the current form
document.querySelector('[data-apos-form-thank-you]')grabs the first element in the DOM; multiple forms will interfere with each other.- const thankYou = document.querySelector('[data-apos-form-thank-you]'); + const thankYou = form.closest('form, .sf-form, body')?.querySelector( + '[data-apos-form-thank-you]', + );Keeps feedback local to the form that was just submitted.
174-188: Optional: add timeout / abort support tofetchVery slow connections will hang indefinitely. Consider an
AbortController:+ const controller = new AbortController(); + const id = setTimeout(() => controller.abort(), 15000); // 15 s return fetch(form.action, { method: 'POST', body: JSON.stringify({ data: formData }), headers: { 'Content-Type': 'application/json', 'Accept': 'application/json', 'CSRF-Token': document .querySelector('meta[name="csrf-token"]') ?.getAttribute('content') || '', }, credentials: 'same-origin', + signal: controller.signal, }).finally(() => clearTimeout(id));Prevents the UI from waiting forever on network issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
website/app.js(2 hunks)website/modules/asset/ui/src/js/formValidation.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- website/app.js
🧰 Additional context used
🪛 Biome (1.9.4)
website/modules/asset/ui/src/js/formValidation.js
[error] 124-124: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: e2e-tests
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: security-scan
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
website/modules/asset/ui/src/js/formValidation.js (1)
162-167:⚠️ Potential issueStill parsing
response.json()without checkingresponse.ok
This was flagged in a previous review and is unresolved.Server errors (4xx / 5xx) or non-JSON responses will explode here, forcing the chain into a generic
catchwith no context. Guard first:-return response - .json() - .then((data) => onHandleServerResponse(data, form)) - .catch(() => false); +if (!response.ok) { + return false; +} +return response + .json() + .then((data) => onHandleServerResponse(data, form)) + .catch(() => false);
🧹 Nitpick comments (2)
website/modules/asset/ui/src/js/formValidation.js (2)
112-125: Nit – simplify with optional chaining & early returnThe triple condition can be shortened and avoids false negatives when whitespace is the only content.
- const error = field - .closest('.sf-field') - ?.querySelector('.validation-error'); - if (error && error.textContent && error.textContent.trim() !== '') { + const error = field + .closest('.sf-field') + ?.querySelector('.validation-error'); + if (error?.textContent?.trim()) {🧰 Tools
🪛 Biome (1.9.4)
[error] 119-119: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
185-190: Prevent double-submits by disabling the form while pendingBecause
handleFormSubmitis async but does not block the UI, rapid clicks can fire multiple simultaneous requests. Disable the submit button(s) until the promise settles to avoid duplicate submissions.Example approach:
const buttons = form.querySelectorAll('[type="submit"]'); buttons.forEach((b) => (b.disabled = true)); validateForm(...) .then(...) .finally(() => buttons.forEach((b) => (b.disabled = false)));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
website/modules/asset/ui/src/js/formValidation.js(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
website/modules/asset/ui/src/js/formValidation.js (3)
website/modules/asset/ui/src/js/domHelpers.js (4)
form(45-45)form(54-54)errorMessage(47-47)errorMessage(56-56)website/modules/asset/ui/src/js/formValidator.js (1)
form(11-11)website/modules/asset/ui/src/js/formValidation.test.js (2)
form(12-12)errorMessage(14-14)
🪛 Biome (1.9.4)
website/modules/asset/ui/src/js/formValidation.js
[error] 119-119: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: e2e-tests
- GitHub Check: security-scan
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: Analyze (javascript-typescript)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
website/modules/asset/ui/src/js/formValidation.js (1)
185-208:⚠️ Potential issueFile / complex-value inputs are silently discarded – keep
FormDataall the way
sendFormDataconvertsFormData→ plain object →JSON.stringify.
File,Blob, and evenDateobjects become{}(or[object File]), so uploads or rich values never reach the server.-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; - } - } - - 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', - }); -}; +const sendFormData = (form, formData) => + fetch(form.action, { + method: 'POST', + body: formData, // let the browser set multipart boundary + headers: { + 'Accept': 'application/json', + 'CSRF-Token': + document + .querySelector('meta[name="csrf-token"]') + ?.getAttribute('content') || '', + }, + credentials: 'same-origin', + });Keeps files intact, removes a needless loop, and shrinks payload construction.
Server already expectsmultipart/form-dataper previous backend change notes.
[FYI: the earlier review comment on this exact issue is still outstanding.]
🧹 Nitpick comments (2)
website/modules/asset/ui/src/js/formValidation.js (2)
62-66: Skip disabled fields during form-wide validationDisabled controls are non-submittable and shouldn’t block form submission.
Consider:- 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"]):not(:disabled), textarea:not(:disabled), select:not(:disabled)', + );
125-132: Log swallowed network errors for easier debuggingThe catch block now shows a UI message (good) but still drops the actual error.
Add a console log so failures aren’t invisible in DevTools:- .catch(() => { + .catch((err) => { + /* eslint-disable no-console */ + console.error('Form submission failed', err);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
website/modules/@apostrophecms/form-widget/views/widget.html(3 hunks)website/modules/asset/ui/src/js/formValidation.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- website/modules/@apostrophecms/form-widget/views/widget.html
🧰 Additional context used
🪛 Biome (1.9.4)
website/modules/asset/ui/src/js/formValidation.js
[error] 158-158: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: e2e-tests
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: security-scan
🔇 Additional comments (1)
website/modules/asset/ui/src/js/formValidation.js (1)
166-183:response.json()will throw on empty 204/205 responsesCalling
response.json()unconditionally after a successful status will reject
for empty bodies, forcing the.catch()path and showing a generic error.Guard for 204/205 or zero
content-length:if (!response.ok) { … } - return response.json().then((data) => onHandleServerResponse(data, form)); + if (response.status === 204 || response.headers.get('content-length') === '0') { + return onHandleServerResponse({}, form); + } + return response.json().then((data) => onHandleServerResponse(data, form));
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
website/modules/asset/ui/src/js/formValidation.js (1)
99-115:⚠️ Potential issueFile / complex-field values still lost – stop converting
FormDatato JSONPrevious feedback highlighted that stringifying a
File,Blob, or multi-value field turns it into[object File]/{}and breaks uploads.
collectFormData(line 99) now returns nativeFormData, butsendFormData(lines 186-208) immediately flattens it back to a plain object and sends JSON, re-introducing the same problem.-const formData = collectFormData(form); -... -return fetch(form.action, { - method: 'POST', - body: JSON.stringify({ data }), - headers: { - 'Content-Type': 'application/json', - ... - }, - credentials: 'same-origin', -}); +const formData = collectFormData(form); + +return fetch(form.action, { + method: 'POST', + body: formData, // let the browser set multipart boundary + headers: { + 'Accept': 'application/json', + 'CSRF-Token': + document.querySelector('meta[name="csrf-token"]')?.getAttribute('content') || '', + }, + credentials: 'same-origin', +});This keeps files intact, handles duplicate field names natively, and removes the fragile manual loop.
Also applies to: 185-208
🧹 Nitpick comments (4)
website/modules/asset/ui/src/js/formValidation.js (4)
166-183: Guard against 204 / empty JSON responses
response.json()will throw on a 204 No Content or on responses without a valid JSON body, pushing the flow into the generic catch and hiding the real status. Check for a body before parsing:- return response.json().then((data) => onHandleServerResponse(data, form)); + if (response.status === 204) { + return onHandleServerResponse({}, form); + } + + return response + .text() + .then((txt) => (txt ? JSON.parse(txt) : {})) + .then((data) => onHandleServerResponse(data, form));
148-156:thankYouselector is global – breaks with multiple forms
document.querySelector('[data-apos-form-thank-you]')always picks the first matching element.
If the page hosts more than one form, only the first “thank-you” banner will be shown/hidden.Consider scoping the search to the current form container:
-const thankYou = document.querySelector('[data-apos-form-thank-you]'); +const thankYou = form.closest('[data-apos-form-wrapper]')?.querySelector('[data-apos-form-thank-you]');
62-66: Skip disabled fields during validationDisabled inputs are currently included in
fields, wasting cycles and potentially flagging irrelevant errors.-'input:not([type="submit"]):not([type="button"]):not([type="hidden"]), textarea, select' +'input:not([type="submit"]):not([type="button"]):not([type="hidden"]):not([disabled]), textarea:not([disabled]), select:not([disabled])'
118-133: Log submission failures for easier debuggingThe catch block shows a user-facing message but discards the actual error object. A console log (behind
eslint-disable no-console) keeps dev insight without affecting UX.-.catch(() => { +.catch((err) => { + /* eslint-disable no-console */ + console.error('Form submission failed:', err);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
website/modules/asset/ui/src/js/formValidation.js(2 hunks)
⏰ 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
|
VitalyyP
changed the title
VitalyyP
changed the title
Pull request was closed
4 participants
Uh oh!
There was an error while loading. Please reload this page.