Skip to content

feat: convert AlertModal to TypeScript#4142

Draft
brian-smith-tcril wants to merge 3 commits intoopenedx:release-23.xfrom
brian-smith-tcril:alertmodal-to-ts
Draft

feat: convert AlertModal to TypeScript#4142
brian-smith-tcril wants to merge 3 commits intoopenedx:release-23.xfrom
brian-smith-tcril:alertmodal-to-ts

Conversation

@brian-smith-tcril
Copy link
Contributor

@brian-smith-tcril brian-smith-tcril commented Feb 24, 2026

Part of #3739 (AlertModal item)

Summary

Converts AlertModal from .jsx to .tsx:

  • Removes propTypes and defaultProps
  • Adds AlertModalProps interface with JSDoc comments
  • Moves defaults into function parameter destructuring
  • Updates src/index.ts to move AlertModal into the typed exports section

Testing

  • npm run type-check passes clean
  • All existing AlertModal tests pass

Log

paragon JSX→TSX conversion (issue #3739)

Background

openedx/paragon is a React component library used across Open edX MFEs. Issue #3739 is a tracking issue to convert all remaining .jsx components to .tsx — eliminating deprecated propTypes/defaultProps APIs, clearing the path to React 19, and giving consumers build-time type errors.

Components are sorted by usage count. AlertModal has ~30 uses and was self-assigned by @brian-smith-tcril. The work involves converting one component per PR (the project asks for 1–3 components per PR to keep review manageable).

We started this session by: reading the issue, identifying the component, cloning the fork, and following the patterns established by earlier merged PRs (Spinner #3714, Badge/Breadcrumb #3807, PageBanner/Menu #3831, etc.).

Repo: /home/bsmith/code/claude/paragon
Branch: alertmodal-to-ts (off release-23.x)
Remotes: origin = SSH (brian-smith-tcril fork), upstream = https (openedx/paragon, no push)

Shell setup

nvm is not on PATH in Bash tool sessions — must source it first:

source ~/.nvm/nvm.sh && nvm use && <command>

Conversion pattern

  1. Rename .jsx.tsx, remove PropTypes and defaultProps
  2. Write an interface <Component>Props { ... } with JSDoc comments from propTypes
  3. Move defaults from defaultProps into function parameter destructuring
  4. PropTypes.nodeReact.ReactNode, PropTypes.elementTypeReact.ComponentType (not React.ElementType — Icon's .d.ts explains why)
  5. Delete the old .jsx file
  6. In src/index.ts: move export from "no types" section (remove // @ts-ignore) into "has types" section (preserve alphabetical order)

Example conversions to reference

  • PageBanner: src/PageBanner/index.tsx
  • Breadcrumb: src/Breadcrumb/index.tsx
  • Badge: src/Badge/index.tsx (uses forwardRef + Bootstrap base)
  • ModalDialog: src/Modal/ModalDialog.tsx (already converted, good reference for Modal-family props)

Known issue: pre-commit hook crashes on Node 24

lint-staged uses npx eslint (see package.json lint-staged config) while CI uses npm run eslinteslint ... .. On Node 24, npx eslint crashes with:

TypeError: Error while loading rule 'react/prefer-stateless-function': `[[GeneratorState]]` is not present on `O`

This is a Node 24 incompatibility in es-iterator-helpers (used by eslint-plugin-react). The full npm run eslint passes clean. Workaround: git commit --no-verify. Issue filed: #4141

AlertModal specifics

  • File: src/Modal/AlertModal.tsx (was .jsx)
  • Wraps ModalDialog — all ModalDialog props passed explicitly (no ...props spread to ModalDialog)
  • onClose is optional in AlertModal (defaults to () => {}), required in ModalDialog — default handles this
  • isOverflowVisible not in original propTypes but needed by ModalDialog; added as optional, defaults to false
  • icon type: React.ComponentType (not React.ElementType) per src/Icon/index.d.ts
  • Branch naming: no prefixes (e.g. alertmodal-to-ts not feat/alertmodal-to-ts)
  • Committed with --no-verify (pre-commit hook broken on Node 24, see above)
  • Branch pushed to origin: https://github.com/brian-smith-tcril/paragon/pull/new/alertmodal-to-ts

🤖 Generated with Claude Code

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@netlify
Copy link

netlify bot commented Feb 24, 2026

Deploy Preview for paragon-openedx-v23 ready!

Name Link
🔨 Latest commit dea6dda
🔍 Latest deploy log https://app.netlify.com/projects/paragon-openedx-v23/deploys/699f5b9dff00b1000854622b
😎 Deploy Preview https://deploy-preview-4142--paragon-openedx-v23.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@brian-smith-tcril
Copy link
Contributor Author

The original onClose prop used requiredWhenNot(PropTypes.func, 'isBlocking') — a runtime PropTypes warning that fired if onClose was omitted when isBlocking was false. However, defaultProps always supplied onClose: () => {}, so the behavior was never actually enforced at runtime.

TypeScript can't express "required when another prop is falsy" without overloads or a discriminated union, which felt like overkill for what was just a warning. Making it optional with a default of () => {} preserves identical runtime behavior.

@codecov
Copy link

codecov bot commented Feb 24, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.42%. Comparing base (dfc69e2) to head (dea6dda).
⚠️ Report is 7 commits behind head on release-23.x.

Additional details and impacted files
@@               Coverage Diff                @@
##           release-23.x    #4142      +/-   ##
================================================
+ Coverage         94.39%   94.42%   +0.03%     
================================================
  Files               242      242              
  Lines              4296     4303       +7     
  Branches           1020     1025       +5     
================================================
+ Hits               4055     4063       +8     
+ Misses              237      232       -5     
- Partials              4        8       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@brian-smith-tcril brian-smith-tcril marked this pull request as draft February 25, 2026 18:06
- Use React.ComponentProps<typeof ModalDialog> + Omit to restore the
  original ...props passthrough to ModalDialog
- Redeclare onClose as optional (AlertModal defaults it to () => {})
- Redeclare isOverflowVisible as optional with default false (was absent
  from original propTypes, undefined is falsy)
- Redeclare variant without 'dark' (intentionally excluded in original)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@brian-smith-tcril
Copy link
Contributor Author

Review updates log

AlertModal PR Review Session

PR: #4142

Issues found

1. Lost ...props passthrough (major)

The original spread ...props to ModalDialog, passing through any props not explicitly handled by AlertModal. The first version of the tsx file replaced this with an explicit prop list, silently dropping any passthrough.

Fix: Use React.ComponentProps<typeof ModalDialog> to derive ModalDialog's prop types (its Props interface is not exported), extend it via Omit, and restore the ...props spread to ModalDialog. Only destructure props that need AlertModal-specific defaults or special handling.

2. isOverflowVisible redeclared as optional

Required in ModalDialog (boolean, no ?) but absent from AlertModal's original propTypes — callers who didn't pass it got undefined flowing to ModalDialog. Since undefined is falsy, defaulting to false matches the previous behavior. Added to Omit and redeclared as isOverflowVisible?: boolean with = false in the function signature.

Note: this is a case where the old JSX code had a silent bug (PropTypes only validates at a component's own boundary, not what it passes to children). Worth raising in the PR.

3. variant redeclared without 'dark'

ModalDialog's variant includes 'dark', but AlertModal's original propTypes only had 'default' | 'warning' | 'danger' | 'success'.

Investigation: 'dark' was in ModalDialog since its initial commit (April 2021). AlertModal's variants were added two years later (April 2023, 4bdc4690) and still didn't include 'dark'. Given the gap and AlertModal's semantic purpose as an alert dialog, this strongly suggests the exclusion was intentional — but we can't say for certain.

Fix: Added variant to the Omit and redeclared without 'dark'.

4. onClose / requiredWhenNot (minor, documented)

Original used requiredWhenNot(PropTypes.func, 'isBlocking') — a runtime-only warning. Since defaultProps always supplied () => {}, it was never enforced. Making it optional with a default preserves identical runtime behavior. Comment added to PR: #4142 (comment)

Final interface shape

type ModalDialogProps = React.ComponentProps<typeof ModalDialog>;

// Extends all ModalDialog props, but omits certain props to re-declare them:
// - onClose: ModalDialog requires it, but AlertModal defaults it to () => {}
// - isOverflowVisible: required in ModalDialog but was absent from AlertModal's propTypes;
//   undefined is falsy so defaulting to false matches the previous behavior
// - variant: ModalDialog allows 'dark' but AlertModal intentionally excludes it
// footerNode and icon are AlertModal-specific props not present on ModalDialog
interface AlertModalProps extends Omit<ModalDialogProps, 'onClose' | 'isOverflowVisible' | 'variant'> {
  onClose?: () => void;
  isOverflowVisible?: boolean;
  variant?: 'default' | 'warning' | 'danger' | 'success';
  footerNode?: React.ReactNode;
  icon?: React.ComponentType;
}

function AlertModal({
  children, footerNode = null, icon, title,
  hasCloseButton = false, onClose = () => {}, isOverflowVisible = false, className,
  ...props
}: AlertModalProps) {
  return (
    <ModalDialog
      {...props}
      title={title}
      hasCloseButton={hasCloseButton}
      onClose={onClose}
      isOverflowVisible={isOverflowVisible}
      className={classNames('pgn__alert-modal', className)}
    >

@brian-smith-tcril
Copy link
Contributor Author

Comment written by Claude Code on behalf of @brian-smith-tcril

isOverflowVisible usage across the openedx org

Searched the openedx org for all <AlertModal call sites to inform whether isOverflowVisible should be optional or required. 45 usages found across 44 files (1 file has multiple usages).

File Passes isOverflowVisible
frontend-app-account · ConfirmationModal.jsx
frontend-app-admin-console · ConfirmDeletionModal.tsx
frontend-app-admin-portal · BudgetExpiryAlertAndModal/index.jsx
frontend-app-admin-portal · BulkEnrollmentWarningModal.jsx
frontend-app-admin-portal · BulkEnrollmentSubmit.jsx
frontend-app-admin-portal · DeleteHighlightSet.jsx
frontend-app-admin-portal · ContentHighlightStepper.jsx
frontend-app-admin-portal · FormWaitModal.tsx
frontend-app-admin-portal · ContentNotInCatalogErrorAlertModal.jsx
frontend-app-admin-portal · NotEnoughBalanceAlertModal.jsx
frontend-app-admin-portal · SystemErrorAlertModal.jsx
frontend-app-admin-portal · GroupMembersCsvDownloadTableAction.jsx
frontend-app-admin-portal · ConfigErrorModal.tsx
frontend-app-admin-portal · DisableLinkManagementAlertModal.jsx
frontend-app-admin-portal · LinkDeactivationAlertModal.jsx
frontend-app-admin-portal · SyncHistory.jsx
frontend-app-admin-portal · ExistingCard.jsx
frontend-app-authoring · ModalError.jsx
frontend-app-authoring · EnableHighlightsModal.jsx
frontend-app-authoring · InfoModal.jsx
frontend-app-authoring · DeleteModal.jsx
frontend-app-authoring · CustomPageCard.jsx
frontend-app-authoring · EditProblemView/index.jsx
frontend-app-authoring · DeleteConfirmationModal.jsx
frontend-app-authoring · DeleteModal.tsx
frontend-app-authoring · modal-notification/index.jsx
frontend-app-authoring · UnlinkModal.tsx
frontend-app-authoring · AppList.jsx
frontend-app-authoring · ManageOrgsModal.jsx
frontend-app-communications · TaskAlertModal.jsx
frontend-app-discussions · TinyMCEEditor.jsx
frontend-app-instruct · AlertProvider.tsx
frontend-app-learner-portal-enterprise · AppErrorBoundary.tsx
frontend-app-learner-portal-enterprise · budget-expiry-notification/index.jsx
frontend-app-learner-portal-enterprise · EnrollModal.jsx
frontend-app-learner-portal-enterprise · custom-expired-subscription-modal/index.jsx
frontend-app-learner-portal-enterprise · SubscriptionExpirationModal.jsx
frontend-app-learner-portal-enterprise · SubscriptionExpirationModal.jsx
frontend-app-learner-portal-enterprise · UnenrollModal.jsx
frontend-app-learner-portal-enterprise · IntegrationWarningModal.jsx
frontend-app-learning · AccountActivationAlert.jsx
frontend-app-ora · ConfirmDialog/index.jsx
frontend-app-ora-grading · ConfirmModal.jsx
frontend-app-ora-grading · DemoAlert/index.jsx
frontend-app-support-tools · CourseReset.jsx

3 of 45 usages explicitly pass isOverflowVisible (the remaining 42 rely on the default).

This confirms that keeping isOverflowVisible optional with a false default is the right call — the vast majority of callers never set it.

Generated by Claude Code

Comment on lines +11 to +12
// - isOverflowVisible: required in ModalDialog but was absent from AlertModal's propTypes;
// undefined is falsy so defaulting to false matches the previous behavior
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think defaulting to false here (as opposed to making this a required prop) is the right call for this PR (see #4142 (comment))

Cover the onClose, footerNode, and isOverflowVisible default parameter
branches, and the no-footer render path, to restore 100% coverage.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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.

1 participant