refactor(modal): migrate Modal from Flow to TypeScript - #4771
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe modal components now have TypeScript implementations with typed props and exports. ChangesModal component migration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Modal
participant DocumentBody
participant LoadingIndicator
participant ModalDialog
Modal->>DocumentBody: suppress body scrolling while open
Modal->>Modal: initialize or trap focus
alt loading
Modal->>LoadingIndicator: render loading state
else loaded
Modal->>ModalDialog: render dialog props and content
end
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/components/modal/Modal.tsx (1)
159-162: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider typing the wrapper instead of casting to
React.ElementType.The
as React.ElementTypecast removes prop checking for both branches. TypeScript then cannot verify thatPortalacceptsonKeyDownandtabIndex.Portalspreads all props onto an innerdiv(src/components/portal/Portal.tsxlines 39-43), so the runtime behavior is correct today. A narrower union keeps the check active.♻️ Alternative typing
- const WrapperComponent = (shouldNotUsePortal ? 'div' : Portal) as React.ElementType; + const WrapperComponent: React.ElementType<React.HTMLAttributes<HTMLDivElement>> = shouldNotUsePortal + ? 'div' + : Portal;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/modal/Modal.tsx` around lines 159 - 162, Update the WrapperComponent typing in the Modal render path to use a narrowed union of the intrinsic div element and the Portal component instead of casting to React.ElementType. Preserve the existing conditional selection and props, while retaining TypeScript validation that onKeyDown, tabIndex, className, and related props are accepted by both branches.src/components/modal/Modal.js.flow (1)
132-176: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffThe
.js.flowshims duplicate full implementations. Both files copy the entire component body from their TSX counterparts. Flow reads only the exported types from a.js.flowfile, so the method bodies serve no purpose and create two sources of truth. Each future change to a TSX file must be mirrored by hand, or Flow consumers receive a stale contract.Confirm the established pattern in this repository before changing these files. Other
.js.flowshims may already use one style consistently.
src/components/modal/Modal.js.flow#L132-L176: replace the class body with adeclare export default class Modal extends React.Component<Props>declaration that keeps thedialogfield.src/components/modal/ModalDialog.js.flow#L39-L170: replace the class body with declarations for the default export and theModalDialogBasenamed export.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/modal/Modal.js.flow` around lines 132 - 176, Replace the duplicated implementations in src/components/modal/Modal.js.flow lines 132-176 and src/components/modal/ModalDialog.js.flow lines 39-170 with Flow declarations, following the repository’s established .js.flow shim pattern. In Modal.js.flow, declare the default Modal class extending React.Component<Props> and retain its dialog field; in ModalDialog.js.flow, declare both the default export and the named ModalDialogBase export without method bodies.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/modal/ModalDialog.tsx`:
- Line 115: Restore the explicit `#909090` color prop on IconClose in
src/components/modal/ModalDialog.tsx at lines 115-115, and keep the matching
color prop in src/components/modal/ModalDialog.js.flow at lines 107-107 so both
implementations render identically.
- Around line 109-114: Add type="button" to the close button rendered by the
ModalDialog component, alongside its existing aria-label and className
attributes. Keep closeButtonProps spread first so callers can override the
default type, matching the behavior already implemented by renderBackButton.
- Around line 184-187: Export the InjectedModalDialogProps type from
ModalDialog.tsx, then update the modal index barrel to re-export
InjectedModalDialogProps instead of or alongside ModalDialogProps so consumers
can type the injected component without supplying intl or closeButtonProps.
---
Nitpick comments:
In `@src/components/modal/Modal.js.flow`:
- Around line 132-176: Replace the duplicated implementations in
src/components/modal/Modal.js.flow lines 132-176 and
src/components/modal/ModalDialog.js.flow lines 39-170 with Flow declarations,
following the repository’s established .js.flow shim pattern. In Modal.js.flow,
declare the default Modal class extending React.Component<Props> and retain its
dialog field; in ModalDialog.js.flow, declare both the default export and the
named ModalDialogBase export without method bodies.
In `@src/components/modal/Modal.tsx`:
- Around line 159-162: Update the WrapperComponent typing in the Modal render
path to use a narrowed union of the intrinsic div element and the Portal
component instead of casting to React.ElementType. Preserve the existing
conditional selection and props, while retaining TypeScript validation that
onKeyDown, tabIndex, className, and related props are accepted by both branches.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 045cb637-38dc-4bd2-9817-93bca8d9e820
📒 Files selected for processing (14)
src/components/modal/Modal.js.flowsrc/components/modal/Modal.tsxsrc/components/modal/ModalActions.js.flowsrc/components/modal/ModalActions.tsxsrc/components/modal/ModalDialog.js.flowsrc/components/modal/ModalDialog.tsxsrc/components/modal/__tests__/Modal.test.tsxsrc/components/modal/__tests__/ModalActions.test.tsxsrc/components/modal/__tests__/ModalDialog.test.tsxsrc/components/modal/index.js.flowsrc/components/modal/index.tssrc/components/modal/stories/Modal.stories.tsxsrc/components/modal/stories/ModalActions.stories.tsxsrc/components/modal/stories/ModalDialog.stories.tsx
💤 Files with no reviewable changes (3)
- src/components/modal/stories/ModalActions.stories.tsx
- src/components/modal/stories/ModalDialog.stories.tsx
- src/components/modal/stories/Modal.stories.tsx
bonchevskyi
force-pushed
the
refactor/flow-to-ts-modal
branch
from
August 12, 2026 11:31
7496e9e to
4c17a73
Compare
vitali-usik
approved these changes
Aug 14, 2026
tjiang-box
approved these changes
Aug 14, 2026
Contributor
Merge Queue Status
This pull request spent 23 minutes 29 seconds in the queue, including 11 minutes 47 seconds running CI. Required conditions to merge
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Convert Modal components to TypeScript
This PR converts
src/components/modalfrom JavaScript with Flow to TypeScript.Changes
index.jstoindex.tswith type exports.js.flowfiles for backward compatibilityContract
Testing
yarn lint,yarn lint:ts, andflow checkpassSummary by CodeRabbit
New Features
Tests