Fix forms submit response parsing - #1395
Conversation
🦋 Changeset detectedLatest commit: 7b5a3bc The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/auth-atproto
@emdash-cms/blocks
@emdash-cms/cloudflare
@emdash-cms/contentful-to-portable-text
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/plugin-cli
@emdash-cms/plugin-types
@emdash-cms/registry-client
@emdash-cms/registry-lexicons
@emdash-cms/sandbox-workerd
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-atproto
@emdash-cms/plugin-audit-log
@emdash-cms/plugin-color
@emdash-cms/plugin-embeds
@emdash-cms/plugin-field-kit
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
![emdashbot[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
The sandbox disconnected during review and I cannot complete the investigation. I was in the middle of tracing the plugin API response envelope to verify whether server-side validation errors are also broken (they appear to return { error: { details: { errors } } } which the client still won't surface). I need a working sandbox to finish the review.
github-actions
Bot
added
review/awaiting-author
There was a problem hiding this comment.
The approach is sound: the plugin API route framework wraps handler return values in { data: ... }, so the browser client needs to unwrap that envelope. The PR correctly fixes successful form submissions.
However, the change is incomplete with respect to field-level validation errors. The PR tests and description mention handling errors, but:
-
The server never returns validation errors inside
data.errors. WhensubmitHandlerencounters invalid data, it throwsPluginRouteError.badRequest("Validation failed", { errors: result.errors }). The core plugin route handler wraps this in the error envelope ({ error: { code: "BAD_REQUEST", message: "Validation failed", details: { errors: [...] } } }). -
The client doesn't check
res.okor read the error envelope.handleSubmitcallsparseSubmitResponse(await res.json())unconditionally. For a 400 response,parseSubmitResponsereturns the raw{ error: ... }object. The client then findsresult.successandresult.errorsboth undefined and falls through to the generic "Something went wrong. Please try again." message. -
The core Astro route drops
detailson the floor.packages/core/src/astro/routes/api/plugins/[pluginId]/[...path].tscallsapiError(code, message, status)without passingresult.error.details, so even if the client were fixed, the field errors would never reach it. -
The test gives false confidence. It asserts that
parseSubmitResponseunwrapsdata.errors, but the server never produces that shape for validation failures.
Recommended fix (within PR scope): change submitHandler in packages/plugins/forms/src/handlers/submit.ts to return validation failures as data instead of throwing:
if (!result.valid) {
return { success: false, errors: result.errors };
}This keeps the response in the data envelope ({ data: { success: false, errors: [...] } }), so the existing client code — including the new parseSubmitResponse — handles it correctly without touching core.
| redirect?: string; | ||
| errors?: Array<{ field: string; message: string }>; | ||
| }; | ||
| const result = parseSubmitResponse(await res.json()); |
There was a problem hiding this comment.
[needs fixing] parseSubmitResponse correctly unwraps the { data } success envelope, but the caller never checks res.ok before treating the response as success data. When submitHandler encounters validation errors, it throws PluginRouteError.badRequest("Validation failed", { errors: result.errors }), which the core route handler wraps in { error: { code: "BAD_REQUEST", message: "Validation failed", details: { errors: [...] } } }. The client parses that error object, finds no success or errors at top level, and falls through to the generic "Something went wrong. Please try again." message. This means field-level validation errors are still completely broken for end users.
The codebase already follows a response.ok check pattern everywhere else (e.g. parsePublicFormDefinitionResponse, parseApiResponse, and the forms admin UI). The client should do the same.
| const result = parseSubmitResponse(await res.json()); | |
| const body = (await res.json()) as unknown; | |
| if (!res.ok) { | |
| const error = isRecord(body) && isRecord(body.error) ? body.error : null; | |
| const errors = Array.isArray(error?.details?.errors) ? error.details.errors : undefined; | |
| if (errors) { | |
| showErrors(form, errors); | |
| } else { | |
| showStatus(form, error?.message || "Something went wrong. Please try again.", "error"); | |
| } | |
| } else { | |
| const result = parseSubmitResponse(body); | |
| if (result.success) { | |
| clearSavedState(form); | |
| if (result.redirect) { | |
| // prevent xss | |
| if (isSafeRedirectUrl(result.redirect)) { | |
| window.location.href = result.redirect; | |
| } else { | |
| showStatus(form, result.message || "Submitted successfully.", "success"); | |
| form.reset(); | |
| } | |
| } else { | |
| showStatus(form, result.message || "Submitted successfully.", "success"); | |
| form.reset(); | |
| } | |
| } else if (result.errors) { | |
| showErrors(form, result.errors); | |
| } else { | |
| showStatus(form, "Something went wrong. Please try again.", "error"); | |
| } | |
| } |
(Note: this client-side fix alone won't fully resolve the issue because the core Astro route packages/core/src/astro/routes/api/plugins/[pluginId]/[...path].ts currently drops details when calling apiError. The cleanest in-package fix is to change submitHandler to return { success: false, errors: result.errors } instead of throwing, so the response stays in the data envelope and the existing client path handles it.)
| }); | ||
| }); | ||
|
|
||
| it("unwraps field errors from the standard API envelope", () => { |
There was a problem hiding this comment.
[needs fixing] This test gives false confidence. It asserts that parseSubmitResponse unwraps data.errors, but the server never returns field-level validation errors inside the data envelope. The forms plugin's submitHandler throws PluginRouteError.badRequest("Validation failed", { errors: result.errors }) on validation failure, which the core plugin route handler wraps in the error envelope ({ error: { details: { errors } } }).
Since the client doesn't read the error envelope and the core Astro route currently drops details when calling apiError, this test documents behavior that doesn't match production. Replace it with a test for the actual error-envelope path (once res.ok handling is added), or remove it once the error path is properly handled.
github-actions
Bot
added
review/needs-rereview
ascorbic
added
bot:review
There was a problem hiding this comment.
This PR correctly fixes the enhanced @emdash-cms/plugin-forms client so it unwraps the standard EmDash plugin API { data: ... } response envelope before inspecting success, message, redirect, and errors. The approach is sound and the implementation is clean.
What was fixed since the prior review
The previous review identified that validation errors went through the error envelope path (PluginRouteError.badRequest), which meant the new parseSubmitResponse helper would never see them. The author followed the recommendation and changed submitHandler to return { success: false, errors: result.errors } as data. This keeps validation failures inside the { data: ... } success envelope, so the client now correctly displays field-level errors instead of falling through to the generic "Something went wrong" message. The test description was also updated to match the actual server behavior.
What I checked
parseSubmitResponsecorrectly unwraps{ data: { success, message, redirect, errors } }and preserves legacy top-level responses.isRecordsafely narrows the envelope without tripping onnull, arrays, or primitives.submitHandlernow returns validation failures as data, aligning the server shape with what the client expects.- The test suite covers envelope unwrapping, legacy compatibility, validation errors, and guards against recursive unwrapping.
- No AGENTS.md conventions are violated: there are no SQL changes, no new admin UI strings, and the changeset is present and accurate.
What remains (pre-existing, not a regression)
The client still does not check res.ok before parsing. Non-validation errors (404 form not found, 403 spam verification failed, 500 internal error) return the { error: ... } envelope, which parseSubmitResponse passes through unchanged. Because result.success and result.errors are then both undefined, the user sees the generic fallback message rather than the specific server error. This behavior is unchanged from before the PR and is outside the stated scope of this fix.
Overall, the PR is focused, correct, and ready to merge.
2 participants
What does this PR do?
Fixes enhanced
@emdash-cms/plugin-formssubmissions so the browser client unwraps the standard EmDash plugin API{ data: ... }response envelope before checkingsuccess,message,redirect, or fielderrors.Without this, successful form submissions can return HTTP 200 with
data.success: truewhile the rendered form still showsSomething went wrong. Please try again.because the client only checked top-levelsuccess.Closes #
Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runmessages.pochanges except in translation PRs — a workflow extracts catalogs on merge tomain.AI-generated code disclosure
Screenshots / test output
corepack pnpm --filter @emdash-cms/plugin-forms typecheckcurrently fails on existing upstream type errors insrc/index.tsandsrc/public-definition.ts, unrelated to this change.