Migrate domains/CORS config form to Ant Design#7853
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
5c06e5e to
9894285
Compare
There was a problem hiding this comment.
Code Review
Clean migration from Chakra/Formik to antd Form. The overall structure is correct — Form.useForm, Form.List, Form.Item rules, and .unwrap() + try/catch for the mutation are all idiomatic antd patterns. The extraction of isValidURL, containsNoWildcard, containsNoPath, and corsOriginRules into validation.ts is a good move for reuse and testability.
Findings (all non-blocking):
-
Potential
isDirtyflash after saving empty list — if the server normalizes[]tonull, there's a brief window post-save whereisDirtycould incorrectly becometrueuntil the form remounts. See inline comment on line 91. -
key={JSON.stringify(apiSettings)}as remount key — works correctly here, but discards in-progress edits on anyapiSettingschange and serializes on every render. See inline comment on line 129. -
eslint-disableon theisDirtydeps array — valid suppression, but a brief comment explaining whyformis omitted would help. See inline comment on line 73. -
corsOriginRulesbundled withrequired— theif (!value) return Promise.resolve()guard works correctly with the pairedrequiredrule, but couples the two. Fine for this use case. See inline comment on line 56. -
Silent
.trim()behavior removed — minor behavior change from the Yup schema; failing with a validation error instead of auto-trimming is arguably better for a security field. See inline comment on line 45.
The Cypress test coverage looks solid — it covers loading states, empty state, editing, removing, and all validation branches. Good work overall.
| validationSchema={ValidationSchema} | ||
| validateOnChange | ||
| onFinish={handleSubmit} | ||
| key={JSON.stringify(apiSettings)} |
There was a problem hiding this comment.
Using key={JSON.stringify(apiSettings)} as a full remount trigger has two concerns:
- Performance:
JSON.stringifyruns on every render whileapiSettingsis being compared for key changes. For this small object it's negligible, but it's a pattern worth calling out. - In-progress edit loss: If
apiSettingschanges while the user is mid-edit (e.g., a background refetch or polling scenario), the form will silently discard their work. The oldenableReinitializein Formik had the same behavior, but antd'sform.setFieldsValueinside the existinguseEffectthat watchesapiSettingswould be a softer reset — only updating when!isDirty.
Not a blocking issue for this PR since there's no background polling here, but worth keeping in mind.
| try { | ||
| await putConfigSettingsTrigger(payload).unwrap(); | ||
| message.success("Domains saved successfully"); | ||
| setBaseline({ cors_origins: values.cors_origins ?? undefined }); |
There was a problem hiding this comment.
Potential brief isDirty = true flash after saving an empty list.
If the user removes all domains and saves, values.cors_origins is [], so setBaseline({ cors_origins: [] }) is called. That's fine. But when the subsequent refetch resolves, the server may return cors_origins: null (or omit it), causing useEffect(() => setBaseline(apiSettings)) to set baseline = { cors_origins: null }. Meanwhile, form.getFieldsValue(true) returns { cors_origins: [] } for an empty Form.List. isEqual([], null) is false, so isDirty briefly becomes true until the key remount re-initializes the form.
In practice the key remount likely resolves this within the same render cycle, but if the server roundtrip is slow you could briefly see Save re-enabled after a successful save. Could be addressed by normalizing null/undefined/[] to the same representation before comparison, or simply by not calling setBaseline on success and letting the refetch/remount handle it.
| () => !isEqual(form.getFieldsValue(true), baseline), | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| [allValues, baseline], | ||
| ); |
There was a problem hiding this comment.
The eslint-disable suppression here is valid — form is intentionally omitted from deps because it's a stable ref from Form.useForm(), and allValues is the actual reactive signal used to trigger re-computation. The pattern is correct, but a short comment explaining why form is omitted would help future readers understand this isn't a forgotten dep.
| { | ||
| validator: (_: unknown, value: string) => { | ||
| if (!value) { | ||
| return Promise.resolve(); |
There was a problem hiding this comment.
The if (!value) return Promise.resolve() guard is correct for the antd rules pattern — when the field is empty, the { required: true } rule above handles the error display and this custom validator cleanly defers to it.
One thing to be aware of: this means corsOriginRules should always be used with the required rule included. If someone imports just the custom validator from corsOriginRules in a non-required context (e.g., an optional URL field), empty string would silently pass. Since the rules are exported as a bundled array that includes required, this is safe for now, but worth noting in a comment if the intent is to keep this reusable.
| } | ||
| try { | ||
| const url = new URL(value); | ||
| return url.pathname === "/" && !value.endsWith("/"); |
There was a problem hiding this comment.
Subtle behavior change from the original Yup schema: the old schema had .trim(), which silently stripped leading/trailing whitespace from values before validation. The new implementation does not trim, so "https://example.com " (trailing space) will fail isValidURL (since new URL rejects invalid hostnames with spaces) with a generic "must be a valid URL" message rather than succeeding silently after trimming.
For a security-sensitive field like CORS origins, rejecting rather than silently normalizing is actually the safer behavior. Just flagging in case the team has an expectation of whitespace tolerance.
Ticket ENG-3182
Description Of Changes
Migrates the domains/CORS configuration page from Chakra UI + Formik to Ant Design + antd Form. Full Formik-to-antd-Form migration including dynamic array fields and custom validation.
What changed:
ChakraBox,ChakraFlex,ChakraText) with Ant Design equivalents (div,Flex,Typography.Paragraph/Typography.Text)Formik,FieldArray, Yup validation) to antdFormwithForm.useForm,Form.List, andForm.ItemrulesisValidURL,containsNoWildcard,containsNoPath) andcorsOriginRulestovalidation.tsfor reuseCustomTextInput/TextInputwithForm.Item+InputisErrorResultpattern to.unwrap()+ try/catchCode Changes
validation.ts- Added reusableisValidURL,containsNoWildcard,containsNoPathfunctions andcorsOriginRulesarraydomains.tsx- Full Chakra/Formik to antd Form migration, Yup to antd rules, layout to Flex/div + TailwindSteps to Confirm
not-a-url) - verify validation error appearshttps://*.example.com) - verify validation errorhttps://example.com/path) - verify validation errorhttps://example.com) - verify no errorPre-Merge Checklist
CHANGELOG.mdupdated