ENG-3174: Migrate privacy notice forms to Ant Design#7950
Conversation
Replace Formik + Yup + Chakra form components with Ant Design Form, Select, Switch, Input, and Card. Use hidden Form.Items for externally managed fields (children, translations), Form.useWatch for reactivity, and inline validation rules instead of Yup schema. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
gilluminate
requested review from
speaker-ender
and removed request for
a team and
speaker-ender
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review: PR #7950 — Migrate Privacy Notice Forms to Ant Design
Overall this is a clean migration from Formik/Chakra to Ant Design. The component structure is clearer, the RTK Query loading states are now surfaced properly, and the removal of FieldArray simplifies the translation tab logic considerably. A few issues to look at before merging:
Must Fix
1. translationIsOOB goes stale when clicking back to an OOB tab (inline comment on PrivacyNoticeTranslationForm.tsx:130)
handleTabSelected unconditionally resets translationIsOOB to false on every tab click. If a user is on an OOB tab, clicks to a non-OOB tab, then clicks back, the <OOBTranslationNotice> banner is gone. The fix is to derive isOOB from availableTranslations based on the currently active language, rather than from creation-event state.
2. isDirty may be true immediately on load (inline comment on PrivacyNoticeForm.tsx:183-185)
form.getFieldsValue(true) includes the hidden <Input>-backed children and translations fields. On first render those are undefined/"" while initialValues.children and initialValues.translations are arrays, so isEqual returns false before any user interaction — enabling the Save button from the start. Comparing allValues directly against initialValues avoids this.
Suggestions
3. Mutations aren't using .unwrap()
The handleSubmit function uses await mutationTrigger() and checks isErrorResult(result). That pattern is functionally correct for RTK Query's result-object style, but the rest of the codebase wraps mutations in .unwrap() + try/catch. Worth standardising:
try {
if (isEditing) {
await patchNoticesMutationTrigger(valuesToSubmit).unwrap();
} else {
await postNoticesMutationTrigger(values).unwrap();
}
message.success(`Privacy notice ${isEditing ? "updated" : "created"}`);
if (!isEditing) router.push(PRIVACY_NOTICES_ROUTE);
} catch (error) {
message.error(getErrorMessage(error));
}4. .then/.catch in useEffect (inline comment on PrivacyNoticeForm.tsx:176-181)
Only place in the file using promise-chain style — easy to switch to async/await + try/catch for consistency.
5. Redundant as SupportedLanguage casts
handleSelect in AddTranslationMenu casts its already-typed SupportedLanguage parameter again on the call to handleCreateLanguage. Same pattern appears inside handleCreateLanguage's body. Both casts can be removed.
6. translation.title has no validation rules
NoticeFormFields renders a Form.Item for translations.${index}.title with no rules. The old Yup schema didn't validate it either, so this is pre-existing — but worth a conscious decision now that validation is done inline.
7. Arbitrary Tailwind value (inline comment on new.tsx:17)
lg:w-[70%] in both page files — prefer a standard Tailwind fraction like lg:w-3/4.
🔬 Codegraph: connected (46795 nodes)
💡 Write /code-review in a comment to re-run this review.
| (Form.useWatch("translations", form) as | ||
| | NoticeTranslationCreate[] | ||
| | undefined) ?? []; | ||
|
|
There was a problem hiding this comment.
clients/admin-ui/src/features/privacy-notices/PrivacyNoticeTranslationForm.tsx:130
Bug: translationIsOOB is a single flag shared across all tabs — it goes stale when switching back to an OOB tab
translationIsOOB is set to true in handleCreateLanguage when the user adds a language that happens to be in availableTranslations, but handleTabSelected unconditionally resets it to false on every tab click. So:
- User has an OOB translation tab (e.g. English).
- User adds a new non-OOB language —
translationIsOOBis correctlyfalse. - User clicks back to the English tab —
handleTabSelectedfires, setstranslationIsOOB = false, and the<OOBTranslationNotice>banner disappears even though that tab is OOB.
Derive the value from data instead of creation-event state:
const isActiveTabOOB = !!availableTranslations?.some(
(t) => t.language === activeLanguage,
);Pass isActiveTabOOB to <TranslationFormBlock isOOB={isActiveTabOOB} />. This also lets you remove the translationIsOOB state and setTranslationIsOOB calls in handleCreateLanguage.
There was a problem hiding this comment.
Done! Derived isActiveTabOOB from availableTranslations and activeLanguage instead of tracking it as event-driven state. Removed the useState and both setter calls.
|
|
||
| const isDirty = useMemo( | ||
| () => |
There was a problem hiding this comment.
clients/admin-ui/src/features/privacy-notices/PrivacyNoticeForm.tsx:183-185
Potential false dirty on load: getFieldsValue(true) includes the hidden <Input>-backed fields, which may not match initialValues on first render
form.getFieldsValue(true) returns every registered field including the two hidden <Form.Item name="children"> and <Form.Item name="translations"> that back plain <Input> controls. When Ant Design first registers an uncontrolled <Input>, its value is undefined or "", while initialValues.children and initialValues.translations are arrays. This means isEqual(getFieldsValue(true), initialValues) is false immediately on load, so isDirty is true before the user has made any changes — and the Save button is incorrectly enabled.
Consider comparing allValues (the object from useWatch([], form)) directly against initialValues rather than calling getFieldsValue(true), since allValues only reflects fields that have actual <Form.Item> controls with real values, and the hidden Input fields won't interfere:
const isDirty = useMemo(
() => !allValues ? false : !isEqual(allValues, initialValues),
[allValues, initialValues],
);If the hidden-Input approach is intentional for reactivity, you may also need to exclude children and translations from the dirty check since they're managed via setFieldValue and tracked separately.
There was a problem hiding this comment.
Done! Switched to comparing allValues (from useWatch) directly against initialValues instead of getFieldsValue(true).
| const [submittable, setSubmittable] = useState(false); | ||
| useEffect(() => { | ||
| form | ||
| .validateFields({ validateOnly: true }) | ||
| .then(() => setSubmittable(true)) | ||
| .catch(() => setSubmittable(false)); |
There was a problem hiding this comment.
clients/admin-ui/src/features/privacy-notices/PrivacyNoticeForm.tsx:176-181
Style: .then/.catch is the only place in this file that uses promise chaining — prefer async/await + try/catch for consistency
The rest of the codebase and this file use async/await. This is a minor inconsistency but worth standardising:
useEffect(() => {
const check = async () => {
try {
await form.validateFields({ validateOnly: true });
setSubmittable(true);
} catch {
setSubmittable(false);
}
};
check();
}, [form, allValues]);There was a problem hiding this comment.
Done! Switched to async/await for consistency.
| @@ -14,15 +16,15 @@ const NewPrivacyNoticePage = () => ( | |||
| { title: "New privacy notice" }, | |||
| ]} | |||
There was a problem hiding this comment.
clients/admin-ui/src/pages/consent/privacy-notices/new.tsx:17
Style: arbitrary Tailwind value lg:w-[70%] — prefer a standard fraction
w-[70%] is an arbitrary value. Consider lg:w-3/4 (75%) or using lg:max-w-[70%] to match the pattern used in other form pages. Same applies to [id].tsx.
There was a problem hiding this comment.
Dropped the lg: breakpoint since we don't support viewports below 1200px. Now just w-[70%].
- Derive isActiveTabOOB from data instead of stale event state - Compare allValues directly for dirty check instead of getFieldsValue - Switch mutations to .unwrap() + try/catch - Convert .then/.catch to async/await for consistency - Remove redundant SupportedLanguage casts - Drop unnecessary lg: breakpoint on page width Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Ant Design Form only includes fields with Form.Item bindings in onFinish values. Without these, disabled and internal_description were dropped from the submitted payload. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2 participants
Ticket ENG-3174
Description Of Changes
Migrates privacy notice create/edit forms from Formik + Chakra to Ant Design Form. This replaces
Formik,Yup,ControlledSelect,CustomTextInput,CustomSwitch,CustomTextArea, andFormSectionwith native Ant DesignForm,Form.Item,Select,Input,Switch, andCardcomponents.Key patterns used:
Form.Itemelements for externally managed fields (children,translations) souseWatchandgetFieldsValuetrack them reactivelyForm.useWatchfor derived state (dirty checking, submittable validation)rulesonForm.Iteminstead of Yup validation schemakeyprop onFormto remount cleanly when switching between noticesisLoadingfor submit state instead of manualuseStateCode Changes
Formik/Formwith Ant DesignFormandForm.useForminPrivacyNoticeForm.tsxFieldArray/useFormikContextwithForm.useWatchandform.setFieldValueinPrivacyNoticeTranslationForm.tsxNoticeKeyFieldfromuseFormikContexttoForm.useFormInstanceandForm.useWatchValidationSchemafromform.ts, replaced with inlinerulesonForm.ItemBox,Flex,VStack,Stack,FormLabel,Heading) with AntFlex,Card,Typography,Divider, and Tailwind utilitiesdata-testidvalues (select-*instead ofcontrolled-select-*)[id].tsx,new.tsx) to use AntTypographyand Tailwind layoutSteps to Confirm
Pre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works