Migrate custom reports forms to Ant Design#7865
Conversation
- Update Cypress tests to use getAntFormError helper for antd form errors - Delete dead code custom-fields/form/ directory (unused CustomInput + types) - Add loading spinner to report creation submit button - Replace ConfirmationModal with imperative Modal.confirm via useModal - Remove redundant delete state management (deleteIsOpen, reportToDelete, useEffect) 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
|
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review: Custom Reports Forms — Chakra/Formik → Ant Design Migration
Overall this is a clean migration. The form logic is clearer with Form.useForm, the delete confirmation flow is improved with useModal, and removing Chakra useDisclosure in favor of plain useState simplifies the state management. The Cypress test updates correctly reflect the new component structure.
Must Fix
1. Fire-and-forget delete (see inline on CustomReportTemplates.tsx:151)
deleteCustomReportMutationTrigger(id) is not awaited and has no error handler. If the request fails, the UI resets as if delete succeeded while the report still exists on the server. The mutation must use .unwrap() in a try/catch, and the local state reset should only run on success.
2. catch (error: any) (see inline on CustomReportCreationModal.tsx:110)
Use unknown (or omit the annotation entirely — TypeScript 4.4+ defaults to unknown in catch clauses). getErrorMessage already accepts unknown.
3. <Space orientation="vertical"> is not a valid prop (see inline on CustomReportTemplates.tsx:280)
The correct Ant Design prop is direction, not orientation. The incorrect prop is silently ignored and the list renders horizontally.
4. onOpenChange applies template on Cancel (see inline on CustomReportTemplates.tsx:353)
The else branch of onOpenChange calls handleApplyTemplate() unconditionally on every close, including when the user clicks the Cancel button or clicks outside the popover. This is unintended application of a template selection on cancel.
Suggestions
style={{ width: "100%" }}onRadio.Group(inline on line 278): Replace withclassName="w-full"for consistency.zIndex={1400} // below chakra modals(inline on line 361): The comment references Chakra, which is being removed. Track this for cleanup once Chakra is fully gone.useCallbackforhandleSelection/handleReset: Lines 190 and 200 suppressreact-hooks/exhaustive-depsbecause these functions are not memoized. Wrapping them inuseCallbackwould allow the suppressions to be removed.- Missing barrel
index.ts:features/common/custom-reports/has noindex.ts, causing sibling features to import with deep paths. Adding a barrel would make imports stable across future refactors.
| @@ -164,11 +151,27 @@ export const CustomReportTemplates = ({ | |||
| deleteCustomReportMutationTrigger(id); | |||
There was a problem hiding this comment.
Bug: fire-and-forget delete — errors are silently dropped
deleteCustomReportMutationTrigger(id) is not awaited, and there's no error handler. If the DELETE request fails (network error, 403, 404), onSavedReportDeleted() has already been called and local state has already been reset — the UI shows a success state, but the report still exists on the server.
const handleDeleteReport = async (id: string) => {
try {
await deleteCustomReportMutationTrigger(id).unwrap();
if (id === fetchedReport?.id) {
handleReset();
onSavedReportDeleted();
}
} catch (error) {
message.error(getErrorMessage(error, "A problem occurred while deleting the report."));
}
};Note: the early state reset should move inside the try block so it only runs on success.
| value={selectedReportId} | ||
| style={{ width: "100%" }} | ||
| > | ||
| <Space orientation="vertical" className="w-full" size="small"> |
There was a problem hiding this comment.
Invalid orientation prop on <Space> — layout will default to horizontal
<Space> does not have an orientation prop; the correct prop is direction. Because extra props are spread silently, TypeScript won't catch this, but the list will render horizontally instead of vertically:
<Space direction="vertical" className="w-full" size="small">| popoverOnOpen(); | ||
| setPopoverIsOpen(true); | ||
| } else { | ||
| handleApplyTemplate(); |
There was a problem hiding this comment.
onOpenChange calls handleApplyTemplate unconditionally on every close — including when the user clicks Cancel
When the user clicks the Cancel button, handleCancel calls setPopoverIsOpen(false), which triggers onOpenChange(false), which then calls handleApplyTemplate(). This means cancelling silently applies whatever template was selected. The same issue occurs when the popover is dismissed by clicking outside.
One fix: track whether the user explicitly cancelled via a ref, and check it in onOpenChange:
const cancelledRef = useRef(false);
// In handleCancel:
cancelledRef.current = true;
setPopoverIsOpen(false);
// In onOpenChange:
onOpenChange={(open) => {
if (open) {
setPopoverIsOpen(true);
} else if (!cancelledRef.current) {
handleApplyTemplate();
} else {
cancelledRef.current = false;
}
}}| <Radio.Group | ||
| onChange={(e) => handleSelection(e.target.value)} | ||
| value={selectedReportId} | ||
| style={{ width: "100%" }} |
There was a problem hiding this comment.
Inline style on Radio.Group — use Tailwind utility instead
<Radio.Group
onChange={(e) => handleSelection(e.target.value)}
value={selectedReportId}
className="w-full"
>style={{ width: "100%" }} can be replaced with the existing w-full Tailwind class, keeping styles consistent with the rest of the file.
Ticket ENG-3183
Description Of Changes
Migrates the custom reports forms (
CustomReportCreationModalandCustomReportTemplates) from Chakra/Formik to Ant Design, continuing the ongoing antd migration effort.Key changes:
Form+ rules-based validation in the report creation modalFormin the report templates popoveruseDisclosurewithuseStatefor popover/modal state managementConfirmationModal(Chakra) with antduseModal().confirm()for delete confirmationCustomInputcomponent and itsInputTypetypegetAntFormErrorhelper instead ofgetByTestId("error-...")Code Changes
CustomReportCreationModal.tsxfrom Formik/Yup to antd Form with rules-based validationCustomReportTemplates.tsxfrom Formik/Chakra disclosure to antd Form anduseStateclients/admin-ui/src/features/common/custom-fields/form/CustomInput.tsx(unused after migration)clients/admin-ui/src/features/common/custom-fields/form/types.ts(unused after migration)clients/admin-ui/cypress/e2e/datamap-report.cy.tsto usegetAntFormErrorfor validation assertionsSteps to Confirm
Pre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works