Skip to content

ENG-181: Migrate direct modal usage from Chakra to Ant Design#7651

Merged
gilluminate merged 9 commits intomainfrom
gill/ENG-181/migrate-direct-modal-usage-to-ant-2
Mar 17, 2026
Merged

ENG-181: Migrate direct modal usage from Chakra to Ant Design#7651
gilluminate merged 9 commits intomainfrom
gill/ENG-181/migrate-direct-modal-usage-to-ant-2

Conversation

@gilluminate
Copy link
Copy Markdown
Contributor

@gilluminate gilluminate commented Mar 13, 2026

Ticket ENG-181

Description Of Changes

Migrate all 44 components that directly used Chakra modal primitives (ChakraModal, ModalBody, ModalContent, ModalFooter, ModalHeader, ModalOverlay, ModalCloseButton) to Ant Design's Modal component.

Key changes beyond the mechanical migration:

  • Fix broken form submission in 3 modalsDeleteUserModal, NewPasswordModal, and DenyPrivacyRequestModal had <Form> wrapping <Modal> with htmlType="submit" buttons in the footer prop. Because Ant Modal renders footer via React.createPortal, the submit button was outside the <form> DOM tree, silently breaking submission. Fixed by using footer={null} and rendering buttons inside <Form> within the modal children (matching the pattern in CustomReportCreationModal).
  • Introduce MODAL_SIZE constants — New modal-sizes.ts with named widths (md: 576, lg: 768, xl: 992, xxl: 1200) aligned to Ant Design breakpoints. All modals that need non-default widths now use these constants instead of hardcoded values.
  • Remove dead Header/Footer exports from locations/modal.tsx — only HeaderCheckboxRow remains (still used by RegulationModal and SubgroupModal).
  • Fix Cypress selectorconsent-reporting.cy.ts updated from #chakra-modal-consent-lookup-modal to getByTestId("consent-lookup-modal").

This PR does not migrate the shared modal abstractions (StandardDialog, FormModal, FilterModal, ConfirmationModal) — those are a separate effort.

Code Changes

  • clients/admin-ui/src/features/common/modals/modal-sizes.ts — New file with MODAL_SIZE constants
  • clients/admin-ui/src/features/locations/modal.tsx — Removed unused Header and Footer exports
  • clients/admin-ui/cypress/e2e/consent-reporting.cy.ts — Replaced Chakra modal ID selector with data-testid
  • clients/admin-ui/src/features/user-management/DeleteUserModal.tsx — Chakra → Ant Modal, fixed portal form submission bug
  • clients/admin-ui/src/features/user-management/NewPasswordModal.tsx — Chakra → Ant Modal, fixed portal form submission bug
  • clients/admin-ui/src/features/privacy-requests/DenyPrivacyRequestModal.tsx — Chakra → Ant Modal, fixed portal form submission bug, removed hardcoded width
  • clients/admin-ui/src/features/manual-tasks/components/CompleteTaskModal.tsx — Chakra → Ant Modal, width={700}MODAL_SIZE.lg
  • clients/admin-ui/src/features/manual-tasks/components/SkipTaskModal.tsx — Chakra → Ant Modal, width={700}MODAL_SIZE.lg
  • 36 additional modal files — Mechanical Chakra → Ant Modal migration (same pattern: open/onCancel/centered/destroyOnClose/title/footer)

Steps to Confirm

  1. Navigate to Settings > User Management, click a user, click "Delete User" — confirm the modal opens, requires username confirmation, and submits successfully
  2. On the same user page, click "Reset password" — confirm the modal opens and password reset submits successfully
  3. Navigate to Privacy Requests, select a request, click "Deny" — confirm the denial modal opens, requires a reason, and submits successfully
  4. Navigate to Reporting > Data map report, open the Reports dropdown, click "Create report" — confirm the modal opens and allows saving a named report
  5. Navigate to Privacy Requests > Manual Tasks (if available), test Complete Task and Skip Task modals — confirm they open at the correct width
  6. Navigate to Consent Reporting, perform a consent lookup — confirm the lookup modal opens correctly

Pre-Merge Checklist

  • Issue requirements met
  • All CI pipelines succeeded
  • CHANGELOG.md updated
    • Add a db-migration This indicates that a change includes a database migration label to the entry if your change includes a DB migration
    • Add a high-risk This issue suggests changes that have a high-probability of breaking existing code label to the entry if your change includes a high-risk change (i.e. potential for performance impact or unexpected regression) that should be flagged
    • Updates unreleased work already in Changelog, no new entry necessary
  • UX feedback:
    • All UX related changes have been reviewed by a designer
    • No UX review needed
  • Followup issues:
    • Followup issues created
    • No followup issues
  • Database migrations:
    • Ensure that your downrev is up to date with the latest revision on main
    • Ensure that your downgrade() migration is correct and works
      • If a downgrade migration is not possible for this change, please call this out in the PR description!
    • No migrations
  • Documentation:
    • Documentation complete, PR opened in fidesdocs
    • Documentation issue created in fidesdocs
    • If there are any new client scopes created as part of the pull request, remember to update public-facing documentation that references our scope registry
    • No documentation updates required

Made with Cursor

@vercel
Copy link
Copy Markdown
Contributor

vercel bot commented Mar 13, 2026

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

Project Deployment Actions Updated (UTC)
fides-plus-nightly Ready Ready Preview, Comment Mar 17, 2026 9:51pm
1 Skipped Deployment
Project Deployment Actions Updated (UTC)
fides-privacy-center Ignored Ignored Mar 17, 2026 9:51pm

Request Review

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 13, 2026

Greptile Summary

This PR migrates 44 components from Chakra UI modal primitives to Ant Design's Modal component, introduces a MODAL_SIZE constants file, fixes a real form submission bug caused by Ant Design's portal-based footer, and removes dead exports from locations/modal.tsx. The mechanical migration is largely consistent and correct across the bulk of the files.

Key issues found:

  • Stale Formik state in 3 "fixed" modalsDenyPrivacyRequestModal, DeleteUserModal, and NewPasswordModal place <Formik> outside the <Modal>. Because destroyOnHidden only destroys the Modal's children, Formik stays mounted with its previous values. When the modal is reopened, the form fields (denial reason, username confirmation, password fields) are pre-filled with stale data from the prior session. The reference pattern from CustomReportCreationModal — which the PR description cites — actually places Formik inside the Modal so it gets destroyed and reinitialized on each open.

  • Fragile Cypress selectorsdatamap-report.cy.ts (lines 266, 422, 474) and manual-tasks.cy.ts (lines 130, 733) were updated to use .ant-modal-close and .ant-modal-title internal Ant Design CSS classes. The original data-testid attribute was removed from ColumnSettingsModal's close button without adding an equivalent. These selectors will silently break on an Ant Design version bump.

  • ConsentManagementModal width regression — The old code had maxWidth="800px" constraining the rendered width; the new code maps to MODAL_SIZE.xxl (1200px), making this modal noticeably wider than before.

  • div elements for flex layouts — Multiple migrated modals use <div className="flex ..."> in footer and title slots where <Flex> from Ant Design/fidesui should be used per project conventions (rule ecc7d51f).

  • PR size — This PR touches 51 files with ~3,400 total line changes, which significantly exceeds the project's 500-line / 15-file policy limit (rule 32b2f192). While the change is largely mechanical, the size makes thorough review difficult.

Confidence Score: 3/5

  • Generally safe for the bulk of the migration, but three security-adjacent modals (DeleteUser, DenyPrivacyRequest, NewPassword) have a stale form state bug that should be fixed before merge.
  • The majority of the 44-modal migration is correct and mechanical. However, the three explicitly "fixed" modals introduce a regression (Formik outside Modal → stale values on reopen), several Cypress tests now use fragile internal CSS class selectors, and the PR significantly exceeds the size policy. None of these are critical data-loss issues, but the stale form state in the user/privacy-request modals affects correctness of user-facing workflows.
  • Pay close attention to DenyPrivacyRequestModal.tsx, DeleteUserModal.tsx, and NewPasswordModal.tsx (stale Formik state), datamap-report.cy.ts (fragile close-button selectors), and ConsentManagementModal.tsx (effective width change).

Important Files Changed

Filename Overview
clients/admin-ui/src/features/privacy-requests/DenyPrivacyRequestModal.tsx Migrated from Chakra to Ant Modal with form submission fix; however Formik is placed outside the Modal, causing stale form values to persist across open/close cycles.
clients/admin-ui/src/features/user-management/DeleteUserModal.tsx Migrated from Chakra to Ant Modal with form submission fix; same Formik-wraps-Modal issue means stale username confirmation persists when modal is reopened.
clients/admin-ui/src/features/user-management/NewPasswordModal.tsx Migrated from Chakra to Ant Modal with form submission fix; Formik placed outside Modal means password fields retain previous values on re-open.
clients/admin-ui/src/features/common/modals/modal-sizes.ts New file introducing named MODAL_SIZE constants aligned to Ant Design breakpoints; clean and well-documented.
clients/admin-ui/cypress/e2e/datamap-report.cy.ts Three close-button assertions migrated from data-testid to .ant-modal-close:visible (internal Ant Design CSS class), making these tests fragile to Ant Design version upgrades.
clients/admin-ui/cypress/e2e/manual-tasks.cy.ts Modal title assertions updated to use .ant-modal-title CSS class instead of semantic selectors — works but ties tests to internal Ant Design class names.
clients/admin-ui/src/features/configure-consent/ConsentManagementModal.tsx Migrated Chakra to Ant Modal; effective width changed from 800px (maxWidth on ModalContent) to 1200px (MODAL_SIZE.xxl), which may be wider than intended.
clients/admin-ui/src/features/common/custom-reports/CustomReportCreationModal.tsx Formik correctly placed inside Modal (the reference pattern); footer={null} with buttons inside Form ensures proper form submission.

Last reviewed commit: 3713961

},
}}
data-testid="add-modal-content"
wrapProps={{ "data-testid": "add-modal-content" }}
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.

In Ant Design, data-testid placed directly on goes to the inner .ant-modal div, while wrapProps targets the outer .ant-modal-wrap overlay. However, cy.getByTestId(...) (which uses [data-testid=...]) will find both — so this isn't a test breakage, but it is an inconsistency that could cause confusion for .within() scoping.

Personally, I like the look of data-testid more and they achieve the same thing. I think we should stick with data-testid="delete-user-modal". AddIntegrationModal, SubgroupModal, AddManualTaskModal, CompleteTaskModal, and SkipTaskModal would need to be updated.

Copy link
Copy Markdown
Contributor Author

@gilluminate gilluminate Mar 17, 2026

Choose a reason for hiding this comment

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

Are you saying you'd prefer updating all of the tests rather than using the wrapProps? Sorry, not sure I understand.

destroyOnHidden
open={isOpen}
onCancel={closeIfComplete}
title={`${disabled ? "Enable" : "Disable"} Connection`}
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.

Intended title-case?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Fixed

Copy link
Copy Markdown
Contributor

@kruulik kruulik left a comment

Choose a reason for hiding this comment

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

Looks good!

gilluminate and others added 9 commits March 17, 2026 15:46
Made-with: Cursor
Cypress's `be.visible` check fails when `data-testid` is placed directly
on the Ant Design `<Modal>` component because it lands on `.ant-modal-root`
— a zero-height div whose children are all `position: fixed` and taken out
of normal flow.

Switch to `wrapProps={{ "data-testid": "..." }}` which puts the testid on
`.ant-modal-wrap` — a `position: fixed; inset: 0` container that occupies
the full viewport and passes the visibility check.

Affected: CompleteTaskModal, SkipTaskModal, AddManualTaskModal,
AddIntegrationModal, SubgroupModal, IntegrationLinkedSystems

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Ant Design Modal renders the title in a <span> inside .ant-modal-title,
not an <h4> like Chakra Modal did.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Replace column-settings-close-button testid with .ant-modal-close:visible
  (Chakra ModalCloseButton was removed, Ant Design has its own close button)
- Add dragenter/dragover events before drop for column reorder test
  (react-dnd HTML5 backend needs the full DnD event sequence to process
  hover handler where reordering occurs)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Move Formik inside Modal so destroyOnHidden resets form state on close
- Add cy.getAntModalClose() command to centralize .ant-modal-close selector
- Fix ConsentManagementModal width regression (1200px -> 768px via MODAL_SIZE.lg)
- Replace div className="flex" with Flex component across 7 modal files

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@gilluminate gilluminate force-pushed the gill/ENG-181/migrate-direct-modal-usage-to-ant-2 branch from 8e7fcef to ae9e2a6 Compare March 17, 2026 21:46
@gilluminate gilluminate added this pull request to the merge queue Mar 17, 2026
Merged via the queue into main with commit 22869c5 Mar 17, 2026
45 of 46 checks passed
@gilluminate gilluminate deleted the gill/ENG-181/migrate-direct-modal-usage-to-ant-2 branch March 17, 2026 22:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants