Skip to content

fix(dashboard): surface validation errors when the submit button is disabled#4868

Open
grolmus wants to merge 8 commits into
vendurehq:minorfrom
grolmus:mgrolmus/oss-540-react-dashboard-operation-buttons-are-frequently-disabled
Open

fix(dashboard): surface validation errors when the submit button is disabled#4868
grolmus wants to merge 8 commits into
vendurehq:minorfrom
grolmus:mgrolmus/oss-540-react-dashboard-operation-buttons-are-frequently-disabled

Conversation

@grolmus

@grolmus grolmus commented Jun 24, 2026

Copy link
Copy Markdown
Collaborator

Summary

Detail-page forms in the dashboard disable the submit button whenever the form is invalid (disabled={!isDirty || !isValid || isPending}), but there was no on-page indication of why. When a field — very often a custom field whose stored value or generated default fails validation — is invalid, the button silently stayed disabled and the only trace was a console.log. This is the behaviour reported in #4741: "operation buttons are frequently disabled with no on-page prompts."

This PR keeps the disabled-button logic and the backend untouched and instead surfaces the reason.

Root cause

useGeneratedForm builds a zod schema from the entity's input type + custom fields and runs react-hook-form in mode: 'onChange'. An invalid value (e.g. a custom field whose value violates its pattern/min/max/enum) makes formState.isValid false, disabling submit. Field-level errors only render once a field is touched, so on a freshly-loaded edit page nothing was shown — the button just looked broken.

Change

  • New FormErrorSummary (lib/components/shared/form-error-summary.tsx): a destructive Alert listing the fields that block saving, rendered once in the shared PageContentWithOptionalForm, so every detail page (generated and bespoke, e.g. product variants) gets it for free. Flattens nested RHF errors (customFields.*, array paths like stockLevels.0.stockOnHand) and never renders empty while the form is invalid (errors without a message fall back to a translated generic; the reserved root bucket is skipped).
  • Validate on load for existing entities (use-generated-form.tsx): form.trigger() runs once per loaded entity (keyed on entity.id) so pre-existing invalid values surface immediately instead of only after the user pokes a field.
  • Simplified the resolver back to zodResolver(schema) and removed two debug console.logs.
  • Exported FormErrorSummary from the public API (lib/index.ts).

No change to the disabled condition, the mutations, or the backend. A valid form (nothing to save) shows no banner — the button is still correctly disabled with no false alarm.

Test plan

Automatede2e/tests/regression/issue-4741-form-error-summary.spec.ts:

  • Adds a nullable oss540NumericCode product custom field (pattern ^[0-9]*$, empty allowed so other tests are unaffected).
  • On the product create page: a valid name enables Create; entering abc disables it and shows the summary banner with the reason; correcting to 12345 clears the banner and re-enables Create.
  • Passes on this branch; fails on master (the summary banner does not exist there).

Manual (chrome-devtools, before/after screenshots attached on the Linear issue OSS-540): edit page of an entity whose custom field value is invalid — before: Update disabled, no prompt; after: banner + field-level error explain exactly which field and why. Verified a valid page (Zone) shows no banner.

Out of scope / follow-ups

  • The summary uses a humanized field path as a fallback label; resolving the configured (translated) custom-field label in the summary would need the entity type threaded into the shared component. The precise, configured label is already shown next to each offending field.
  • The zod validation messages themselves (Value must match pattern: …, etc., from form-schema-tools.ts) are not yet wrapped for i18n — a pre-existing gap now made more visible. Worth a dedicated follow-up.

Fixes #4741


View with Codesmith Autofix with Codesmith
Need help on this PR? Tag /codesmith with what you need. Autofix is disabled.

grolmus added 2 commits June 24, 2026 13:10
…isabled

Detail-page forms disable the submit button when the form is invalid, but
the reason was never shown on the page (only logged). A field with an invalid
value — typically a custom field whose stored value or default fails
validation — left the button silently disabled.

Add a shared FormErrorSummary banner (rendered for every detail page) that
lists the blocking fields, and validate existing entities on load so
pre-existing invalid values surface immediately instead of only after the
user edits a field.

Fixes vendurehq#4741
Drives a product form into an invalid state via a patterned custom field and
asserts the FormErrorSummary banner appears, the submit stays disabled, and
both clear once the value is corrected. Fails without the fix.

Relates to vendurehq#4741
@vercel

vercel Bot commented Jun 24, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
vendure-storybook Ready Ready Preview, Comment Jul 15, 2026 11:53am

Request Review

@coderabbitai

coderabbitai Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: a7068d73-ea33-4496-809a-e023404320c4

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

A new FormErrorSummary React component is added and rendered in dashboard form layouts. It flattens nested react-hook-form errors, humanizes field paths, and shows a destructive alert with localized summary text and fallback error text. useGeneratedForm now uses zodResolver directly, triggers validation after an entity loads, and removes an invalid-submit log. The component is re-exported from the dashboard barrel, and the PR adds a product custom field fixture plus a Playwright regression test for the new error-summary behavior.

Suggested labels

claude-code-assisted

Suggested reviewers

  • michaelbromley
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main dashboard change: surfacing validation errors for disabled submit buttons.
Description check ✅ Passed The description covers the summary, issue, test plan, and scope well, with only template sections like breaking changes/screenshots/checklist left unfilled.
Linked Issues check ✅ Passed The PR surfaces why operations are disabled across detail pages, which matches the core goal of #4741.
Out of Scope Changes check ✅ Passed The added fixture, i18n strings, form summary, form-triggering, and regression test all stay within the issue's scope.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In
`@packages/dashboard/e2e/tests/regression/issue-4741-form-error-summary.spec.ts`:
- Around line 5-13: Update the header comment in the affected Dashboard E2E spec
to match the required issue-reference format. In the test file near the top of
the regression spec, replace the URL-style multi-line issue note with a single
comment starting with `// `#4741` — ...` and keep the rest of the description
concise. Use the existing `issue-4741-form-error-summary.spec.ts` test and its
`FormErrorSummary`/`oss540NumericCode` context to locate the comment, and ensure
it follows the Dashboard E2E guideline for
`packages/dashboard/e2e/**/*.spec.ts`.

In `@packages/dashboard/src/lib/components/shared/form-error-summary.tsx`:
- Around line 52-55: The humanizeFieldName() helper is stripping all path
context by only using the last segment, which makes repeated or nested fields
indistinguishable. Update form-error-summary.tsx so this function preserves
enough of the original field path to disambiguate entries like
translations.0.name vs translations.1.name while still humanizing the final
label; keep the unique path context in the output and adjust the existing
transformation in humanizeFieldName().

In `@packages/dashboard/src/lib/framework/form-engine/use-generated-form.tsx`:
- Around line 168-179: The validation effect in useGeneratedForm currently runs
only when entity?.id changes, so it can fire before the derived form values are
fully built. Update the useEffect that calls form.trigger() to also wait for the
form shape inputs used to derive values (such as
serverConfig?.availableLanguages and the schema-driven inputs around the values
computation) so it reruns when that shape settles for the same entity. Keep the
one-time-per-entity behavior, but ensure the trigger happens after the generated
values are complete enough to catch hidden invalid translations/custom fields.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 1920dc2c-5caf-4de3-929c-e8e2c08be6d4

📥 Commits

Reviewing files that changed from the base of the PR and between 8850fa7 and 14bbd98.

📒 Files selected for processing (7)
  • packages/dashboard/e2e/fixtures/e2e-shared-config.ts
  • packages/dashboard/e2e/tests/regression/issue-4741-form-error-summary.spec.ts
  • packages/dashboard/src/i18n/locales/en.po
  • packages/dashboard/src/lib/components/shared/form-error-summary.tsx
  • packages/dashboard/src/lib/framework/form-engine/use-generated-form.tsx
  • packages/dashboard/src/lib/framework/layout-engine/page-layout.tsx
  • packages/dashboard/src/lib/index.ts

Comment thread packages/dashboard/e2e/tests/regression/issue-4741-form-error-summary.spec.ts Outdated
Comment thread packages/dashboard/src/lib/components/shared/form-error-summary.tsx Outdated
Comment thread packages/dashboard/src/lib/framework/form-engine/use-generated-form.tsx Outdated
@grolmus grolmus requested a review from michaelbromley June 24, 2026 11:17
…shape settles

Address CodeRabbit review on vendurehq#4868:
- Keep path context in the error summary so repeated/nested fields are
  distinguishable (e.g. "Translations #1 › Name" instead of "Name").
- Re-run the on-load validation when the schema/available languages settle, not
  only on entity id, so hidden invalid custom fields/translations surface even
  if the entity loads before those inputs.
- Use the mandated issue-reference comment format in the regression spec.

Relates to vendurehq#4741
@grolmus

grolmus commented Jun 24, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks @coderabbitai — all three addressed in 2a0aaf1:

  1. Revalidate after the form shape settles (Major): the on-load trigger() now also depends on schema and availableLanguages, so it re-runs if those settle after the entity loads — hidden invalid custom fields/translations no longer stay hidden on first paint.
  2. Disambiguate repeated/nested fields: humanizeFieldName now preserves path context, e.g. translations.0.nameTranslations #1 › Name (array indices shown 1-based; the noise customFields segment dropped).
  3. E2E comment format: switched to the mandated // \[React dashboard] Operation buttons are frequently disabled with no on‑page prompts. #4741` — …` form.

Verified: e2e regression still passes on the branch (and fails without the fix), tsc and check-lib-imports green.

@coderabbitai

coderabbitai Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

@grolmus All three look excellent — especially the schema+availableLanguages dependency in trigger(), which closes a real edge case where late-resolving custom fields could silently stay invalid on first paint. The humanized path format (Translations #1 › Name) is a nice UX touch too. Thanks for the thorough follow-up and for verifying the full CI pipeline!

(∩`-´)⊃━☆゚.*・。゚


If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

@michaelbromley

Copy link
Copy Markdown
Member

@grolmus can you include screenshots and/or screen recording of how this looks?

@grolmus

grolmus commented Jun 25, 2026

Copy link
Copy Markdown
Collaborator Author

@grolmus can you include screenshots and/or screen recording of how this looks?

01-valid-no-banner 02-invalid-banner 02b-banner-closeup 03-fixed-no-banner

@michaelbromley michaelbromley changed the base branch from master to minor June 29, 2026 12:14
@grolmus

grolmus commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator Author

@michaelbromley Ran the vendurehq/design skills as suggested — npx skills add vendurehq/design, then /vendure-ui and /vendure-tokens against this branch.

TL;DR: both pass, no blockers. /vendure-tokens is clean; /vendure-ui flagged only enhancements, and I've applied the one that's actually worth it.

/vendure-tokens — clean

  • Host correctly classified as Vendure-owned → component code must consume published token slots, no raw values.
  • No literal colors, no generic palette ramps. All colour is delegated to the destructive semantic slot via Alert variant="destructive". list-disc pl-4 / font-medium are layout/weight only (typography carries no state colour). Nothing for @vendure-io/design-lint to catch.

/vendure-ui — pass, minor findings

  1. (a11y, medium) — fixed. Summary items were plain text, not linked to the offending field, so they missed the standard WCAG error-summary "skip to field" affordance. Each item is now a button that calls form.setFocus(path), which also scrolls a field that's out of view back into focus — the exact scenario this PR targets.
  2. (low) — left as-is. The banner shifts content when it toggles between header and body. Reserving space would leave a permanent empty gap, which is worse.
  3. (low) — left as-is. Fallback humanized labels (Translations #1 › Name) differ from the configured/translated field labels. Already called out as an out-of-scope follow-up in the PR description; the precise label is still shown next to each field.
  4. (graduation) FormErrorSummary is generic enough to graduate into @vendure-io/ui as a molecule if a second consumer wants it. Noting for later, not doing it here.

Typecheck passes with the change.

@grolmus

grolmus commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator Author
01-summary-full-page 02-summary-banner 03-click-focuses-field

@vendure-ci-automation-bot

vendure-ci-automation-bot Bot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

…ct-dashboard-operation-buttons-are-frequently-disabled
@grolmus

grolmus commented Jul 15, 2026

Copy link
Copy Markdown
Collaborator Author

CI fixes after updating from minor: infinite validation loop + i18n drift

After bringing this branch up to date with the latest minor, two jobs were red: the whole dashboard e2e matrix (all 4 shards) and dashboard i18n sync. Both are now fixed.

1. dashboard e2e — infinite validate → re-render loop on detail pages

Symptom. Dozens of unrelated tests across the suite (product-variants, products/customers CRUD, breadcrumb, translation-placeholders, customer-group…) timed out at 40s, with Playwright reporting the target element "resolved … waiting for element to be visible, enabled and stable" — the signature of a page stuck re-rendering. Importantly, minor alone is green on these specs, so the regression is introduced by this branch's change, not the base.

Root cause. This PR adds an on-load validation effect in useGeneratedForm:

useEffect(() => { if (entity) void form.trigger(); }, [entity?.id, schema, availableLanguages]);

schema is useMemo(() => createFormSchemaFromFields(updateFields, customFieldConfig), [updateFields, customFieldConfig]). The customFieldConfig comes from useCustomFieldConfig(), which returned a new array on every render (.filter(...) ?? [], unmemoised). So schema got a fresh identity every render → the new effect fired every render → form.trigger() updated form state → re-render → new customFieldConfig → new schema → trigger → … an infinite loop on every entity detail page. The page never settled, so every interaction timed out.

The unstable identity was actually a pre-existing latent issue — schema, defaultValues and values were all recomputing every render, and the form-engine memos explicitly document that they assume customFieldConfig is identity-stable. The on-load effect was simply the first consumer to turn that instability into a hang.

Fix. Memoise the return value of useCustomFieldConfig (useMemo(..., [serverConfig, entityType, hasPermissions]); hasPermissions is already useCallback-stable). This restores the identity contract the form engine assumes and repairs every downstream consumer at once — no change to the on-load effect itself. Also added a note on GeneratedFormOptions.customFieldConfig documenting that it must be referentially stable, since that's the remaining entry point to the same loop.

Verification. Local dashboard e2e, same environment, before/after:

  • products.spec.ts — 15/15 pass; product-variants.spec.ts — 13/13 pass. The exact tests that timed out at 40s in CI (e.g. should navigate to manage variants page, should add an option group via the sidebar dialog, force-remove an in-use option group) now pass in 1–3s each.
  • Without the fix, the same spec hangs past 2 minutes.

2. dashboard i18n sync

The two new FormErrorSummary strings ("This cannot be saved until the following are fixed:", "This field is invalid") were only added to en.po; the other locale catalogs never received the msgids. Ran lingui extract and committed the updated .po files (new entries land untranslated, as expected). scripts/check-i18n-sync.sh is clean.

…buttons-are-frequently-disabled

Regenerate i18n catalogs (lingui extract) to resolve .po conflicts
@michaelbromley michaelbromley added the T2: Elevated risk Touches critical paths or has wide blast radius. Needs careful, owned work. label Jul 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

T2: Elevated risk Touches critical paths or has wide blast radius. Needs careful, owned work.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[React dashboard] Operation buttons are frequently disabled with no on‑page prompts.

2 participants