fix(modal): onOpenChange triggering when modal opens #3902
Conversation
🦋 Changeset detectedLatest commit: d20d2f2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@sanuj21 is attempting to deploy a commit to the NextUI Inc Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe changes in this pull request involve modifications to the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
.changeset/early-ghosts-fix.md (1)
6-7: Consider adding more details to the change description.While the current description provides a good overview, it could be enhanced to provide more context and details about the changes. This would help users and maintainers better understand the impact and reasoning behind the modifications.
Consider expanding the description to include:
- The reason for the change (addressing the issue of
onOpenChangenot firing when the modal opens).- The expected impact on users (improved consistency in
onOpenChangebehavior).- Any potential considerations for users updating to this version.
Here's a suggested expanded description:
Added useEffect in useModal to fire onOpenChange when onOpen changes to true. This ensures that onOpenChange is triggered consistently for both opening and closing of the modal. In useDisclosure, onOpenChange now depends on arg onOpen, rather than state(onOpen), improving responsiveness to external state changes. These changes address the issue where onOpenChange was only firing when the modal closed. Users can now rely on onOpenChange for actions related to both opening and closing the modal. This update maintains backwards compatibility while providing more consistent behavior.packages/hooks/use-disclosure/src/index.ts (1)
47-54: Approve changes with suggestions for improvementThe modifications to
onOpenChangealign well with the PR objectives, allowing the callback to fire consistently for both opening and closing actions. This change enhances the flexibility and usability of theuseDisclosurehook.However, to ensure a smooth transition for existing users and maintain type safety, consider the following suggestions:
- Update the TypeScript interface
UseDisclosurePropsto reflect the newonOpenChangesignature:export interface UseDisclosureProps { // ... existing props onOpenChange?(isOpen: boolean): void; }
- Add a comment explaining the new behavior of
onOpenChangefor clarity:/** * Callback fired when the disclosure state changes. * @param isOpen - The new state of the disclosure. */ const onOpenChange = useCallback( (isOpen: boolean) => { // ... existing implementation }, [isOpen, onOpen, onClose] );
- Consider adding a warning or deprecation notice for the old usage pattern to help users transition to the new API.
packages/components/modal/src/use-modal.ts (1)
132-136: LGTM: useEffect hook addresses the issue effectively.The added
useEffecthook successfully addresses the issue by triggeringonOpenChangewhen the modal opens. This implementation is non-breaking and aligns with the PR objectives.Consider a minor optimization:
useEffect(() => { if (isOpen) { onOpenChange?.(isOpen); } -}, [isOpen]); +}, [isOpen, onOpenChange]);Adding
onOpenChangeto the dependency array ensures the effect uses the latest callback if it changes between renders.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- .changeset/early-ghosts-fix.md (1 hunks)
- packages/components/modal/src/use-modal.ts (2 hunks)
- packages/hooks/use-disclosure/src/index.ts (1 hunks)
🧰 Additional context used
🔇 Additional comments (3)
.changeset/early-ghosts-fix.md (1)
1-4: LGTM: Version bumps are appropriate.The minor version bumps for "@nextui-org/modal" and "@nextui-org/use-disclosure" are appropriate for the described changes. This ensures that users of these packages will receive the new functionality while maintaining backwards compatibility.
packages/components/modal/src/use-modal.ts (2)
6-6: LGTM: Import statement updated correctly.The addition of
useEffectto the import statement is necessary and correctly placed with other React imports. This change supports the new functionality added later in the file.
Line range hint
1-236: Summary: Changes effectively address the issue with minimal risk.The modifications successfully ensure that
onOpenChangefires when the modal opens, addressing the reported issue without introducing breaking changes. The implementation is clean and integrates well with the existing code structure.To further improve confidence in this change:
Consider adding a test case to verify the new behavior:
it('should call onOpenChange when modal opens', async () => { const onOpenChange = jest.fn(); const { result } = renderHook(() => useModal({ onOpenChange })); act(() => { result.current.onClose(); // Ensure modal is closed }); act(() => { result.current.isOpen = true; // Simulate opening the modal }); await waitFor(() => { expect(onOpenChange).toHaveBeenCalledWith(true); }); });This test would ensure that the
onOpenChangecallback is called with the correct value when the modal opens.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
.changeset/early-ghosts-fix.md (1)
6-6: LGTM with a minor suggestion: Add missing article.The change description accurately summarizes the modifications and includes the relevant issue reference. However, there's a small grammatical improvement to be made.
Consider this minor change:
-Added useEffect in useModal to fire onOpenChange and in useDisclosure, onOpenChange now accepts onOpen as parameter(#3887) +Added useEffect in useModal to fire onOpenChange and in useDisclosure, onOpenChange now accepts onOpen as a parameter (#3887)🧰 Tools
🪛 LanguageTool
[uncategorized] ~6-~6: You might be missing the article “a” here.
Context: ...ure, onOpenChange now accepts onOpen as parameter(#3887)(AI_EN_LECTOR_MISSING_DETERMINER_A)
packages/components/modal/__tests__/modal.test.tsx (1)
Line range hint
1-143: Overall assessment of changesThe addition of a new test case for the Modal component's opening behavior is a positive step. It demonstrates an effort to improve test coverage and ensure the component behaves correctly when opened.
However, as noted in the previous comment, there are opportunities to enhance this test further:
- Ensure the modal starts in a closed state and verify it opens.
- Use consistent naming for the callback function.
- Verify the correct boolean value is passed to the callback.
These improvements will make the test more robust and better reflect real-world usage of the Modal component.
Consider adding more comprehensive tests that cover different scenarios of modal usage, such as:
- Testing the modal with different content
- Verifying accessibility attributes change correctly when the modal opens and closes
- Testing keyboard navigation within the modal
This will help ensure the Modal component remains reliable and accessible as the codebase evolves.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- .changeset/early-ghosts-fix.md (1 hunks)
- packages/components/modal/tests/modal.test.tsx (2 hunks)
🧰 Additional context used
🪛 LanguageTool
.changeset/early-ghosts-fix.md
[uncategorized] ~6-~6: You might be missing the article “a” here.
Context: ...ure, onOpenChange now accepts onOpen as parameter(#3887)(AI_EN_LECTOR_MISSING_DETERMINER_A)
🔇 Additional comments (3)
.changeset/early-ghosts-fix.md (2)
1-3: LGTM: Appropriate version bumps for the changes.The "patch" updates for both "@nextui-org/modal" and "@nextui-org/use-disclosure" are correct, as they reflect minor bug fixes without introducing breaking changes. This aligns well with the PR objectives.
1-6: Great job addressing previous review comments!The changes suggested in the past review comments have been successfully implemented:
- Both package updates are now set to 'patch'.
- The description is summarized in one line and includes the issue reference ([BUG] - Modal onOpenChange only fires on close #3887) at the end.
These improvements enhance the clarity and consistency of the changeset.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~6-~6: You might be missing the article “a” here.
Context: ...ure, onOpenChange now accepts onOpen as parameter(#3887)(AI_EN_LECTOR_MISSING_DETERMINER_A)
packages/components/modal/__tests__/modal.test.tsx (1)
6-6: LGTM: New import statement for Button component.The import of the Button component is correctly added and is necessary for the new test case.
| test("should fire 'onOpenChange' callback when open button is clicked", async () => { | ||
| const onOpen = jest.fn(); | ||
|
|
||
| const {getByLabelText} = render( | ||
| <> | ||
| <Button aria-label="Open" onClick={onOpen}> | ||
| Open Modal | ||
| </Button> | ||
| <Modal isOpen> | ||
| <ModalContent> | ||
| <ModalHeader>Modal header</ModalHeader> | ||
| <ModalBody>Modal body</ModalBody> | ||
| <ModalFooter>Modal footer</ModalFooter> | ||
| </ModalContent> | ||
| </Modal> | ||
| </>, | ||
| ); | ||
|
|
||
| const openButton = getByLabelText("Open"); | ||
|
|
||
| const user = userEvent.setup(); | ||
|
|
||
| await user.click(openButton); | ||
|
|
||
| expect(onOpen).toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
💡 Codebase verification
ModalProps interface missing onOpenChange prop
The ModalProps interface does not include the onOpenChange prop, which is used in various Modal components and tests. This inconsistency can lead to type errors and unreliable test behavior.
🔗 Analysis chain
Improve test case for modal opening behavior
While the new test case is a good addition, there are a few areas that could be improved:
- The test doesn't actually open the modal or check its state. Consider updating the Modal to start closed and verify it opens.
- The Modal is rendered as always open (
isOpen={true}), which doesn't reflect a real-world scenario. It should start closed. - The test name mentions 'onOpenChange', but the mock function is called 'onOpen'. This should be consistent.
Here's a suggested improvement:
test("should fire 'onOpenChange' callback when modal is opened", async () => {
const onOpenChange = jest.fn();
const { getByLabelText } = render(
<>
<Button aria-label="Open">Open Modal</Button>
<Modal isOpen={false} onOpenChange={onOpenChange}>
<ModalContent>
<ModalHeader>Modal header</ModalHeader>
<ModalBody>Modal body</ModalBody>
<ModalFooter>Modal footer</ModalFooter>
</ModalContent>
</Modal>
</>
);
const openButton = getByLabelText("Open");
const user = userEvent.setup();
await user.click(openButton);
expect(onOpenChange).toHaveBeenCalledWith(true);
});This version:
- Uses a consistent name for the callback (
onOpenChange) - Starts with the modal closed
- Verifies that
onOpenChangeis called withtruewhen the modal is opened
To ensure the Modal component behaves as expected with these changes, we can run the following verification:
This will confirm that the Modal component accepts an onOpenChange prop of the correct type.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the Modal component's onOpenChange prop
ast-grep --lang typescript --pattern 'interface ModalProps {
$$$
onOpenChange?: (isOpen: boolean) => void;
$$$
}'
Length of output: 121
Script:
#!/bin/bash
# Search for occurrences of 'onOpenChange' in the codebase
rg 'onOpenChange' --type=typescript
Length of output: 70
Script:
#!/bin/bash
# Search for similar prop names related to opening the modal
rg 'onOpen[^A-Z]' --type=typescript
Length of output: 72
Script:
#!/bin/bash
# Verify the Modal component's onOpenChange prop with a flexible pattern
ast-grep --lang typescript --pattern 'interface ModalProps {
$$$
onOpenChange?($_): void;
$$$
}'
Length of output: 104
Script:
#!/bin/bash
# Search for occurrences of 'onOpenChange' in .ts and .tsx files
rg 'onOpenChange' --glob '*.ts' --glob '*.tsx'
Length of output: 9539
Script:
#!/bin/bash
# Search for similar prop names related to opening the modal in .ts and .tsx files
rg 'onOpen[^A-Z]' --glob '*.ts' --glob '*.tsx'
Length of output: 4430
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/components/modal/__tests__/modal.test.tsx (1)
114-125: LGTM with suggestion: New test case for onOpenChangeThe new test case is well-implemented and correctly verifies that the
onOpenChangecallback is fired when the modal opens. Good job on usinguserEventfor simulating the button click.Suggestion for improvement:
Consider verifying the argument passed toonOpenChange. You can update the expectation to:expect(onOpenChange).toHaveBeenCalledWith(true);This ensures that
onOpenChangeis not only called but also receives the correct argument indicating that the modal is opening.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- packages/components/modal/tests/modal.test.tsx (2 hunks)
🧰 Additional context used
🔇 Additional comments (2)
packages/components/modal/__tests__/modal.test.tsx (2)
4-6: LGTM: New imports are correctly addedThe new imports for
ButtonanduseDisclosureare necessary for the newly added test case. They are correctly placed and sourced from the appropriate packages.
95-112: LGTM: ModalWrapper component is well-implementedThe ModalWrapper component is correctly implemented:
- It uses the
useDisclosurehook to manage the Modal's open state.- The Button is properly set up to open the Modal.
- The Modal correctly receives the
isOpenandonOpenChangeprops.This component provides a good setup for testing the Modal's opening behavior.
wingkwong
requested review from
ryo-manba
and removed request for
jrgarciadev
wingkwong
changed the title
| useEffect(() => { | ||
| if (isOpen) { | ||
| onOpenChange?.(isOpen); | ||
| } | ||
| }, [isOpen]); | ||
|
|
There was a problem hiding this comment.
I came across this interesting resource on the React docs (https://react.dev/learn/you-might-not-need-an-effect#adjusting-some-state-when-a-prop-changes) that suggests alternative approaches to useEffect in some scenarios. It might be worth considering if it applies to this particular use case.
In the example provided, they achieve a similar outcome by storing information from previous renders. I took a stab at adapting it here (untested):
const [prevIsOpen, setPrevIsOpen] = useState(isOpen);
if (isOpen !== prevIsOpen) {
setPrevIsOpen(isOpen);
if (isOpen) {
onOpenChange?.(isOpen);
}
}What do you think?
There was a problem hiding this comment.
Thanks, It seems a very nice idea to avoid useEffect. I'll test and see if it works.
There was a problem hiding this comment.
I tested, actually the onOpenChange is eventually changing the isOpen state. And we shouldn't directly update parent state inside child without useEffect.
I tried though and got the warning : "Cannot update a component (App) while rendering a different component (NextUI.Modal)".
Let me know if I am missing something.
| const onOpenChange = useCallback( | ||
| (isOpen: boolean) => { | ||
| const action = isOpen ? onOpen : onClose; |
There was a problem hiding this comment.
Since isOpen is managed within useDisclosure whether it is controlled or uncontrolled, is it necessary to pass it as an argument?
There was a problem hiding this comment.
Yes, in this case its needed. Because when the onClose is called its modifiying the internal state as I said earlier ( isOpen becomes false), but at this point the isOpen which is in useDisclosure is still true.
Initially the definition of onOpenChange was like this isOpen ? onClose : onOpen, thats why it was working when onClose used to get called.
But now that will cause infinite render due to useEffect, thats why I reversed it and and added and argument.
There was a problem hiding this comment.
Isn't that the expected behavior?
- You press the close button →
state.close(internal state becomesfalse). onOpenChangeis called (at this point,useDisclosurestate is stilltrue).- Since
isOpenistrue,onClosegets called. - The
useDisclosurestate becomesfalse(syncing the internal state with useDisclosure).
Accepting an argument would be a breaking change, so it might be better to leave it as is for backward compatibility.
There was a problem hiding this comment.
Could you clarify in what cases the useEffect bug occurs?
There was a problem hiding this comment.
Isn't that the expected behavior?
- You press the close button →
state.close(internal state becomesfalse).onOpenChangeis called (at this point,useDisclosurestate is stilltrue).- Since
isOpenistrue,onClosegets called.- The
useDisclosurestate becomesfalse(syncing the internal state with useDisclosure).Accepting an argument would be a breaking change, so it might be better to leave it as is for backward compatibility.
-
You press the open button → external state becomes true. That will remount modal Component.
-
useOverlayTriggerStatewill re-run with isOpentrue. After this,useEffectwill be called and it will call the outer onOpenChange(which was received as prop to modal), without passing argument((useDisclosurestate istruehere). -
Since
isOpenistrue,onClosegets called(at this point,useDisclosurestate becomesfalse, but internal state is still true). (Not sync) -
Now, you press the close button →
state.close(internal state becomesfalse). -
onOpenChangeis called (at this point,useDisclosurestate isfalse), which meansonOpengets called but hereonCloseneeds to get called.
Ya, i thought that it might be a breaking change, but we are also passing state while calling onOpenChange from useOverlayTriggerState
There was a problem hiding this comment.
Oh, so the useEffect added in useModal is the issue. Hmm, with the current implementation, it might introduce new bugs. Could you consider exploring alternative implementation methods?
There was a problem hiding this comment.
What do you think? @jrgarciadev
|
@chirokas |
Closes #3887
📝 Description
The changes in this PR makes the onOpenChange fire when modal opens, making it consistent.
⛳️ Current behavior (updates)
🚀 New behavior
💣 Is this a breaking change (Yes/No):
No
📝 Additional Information
Summary by CodeRabbit
New Features
Bug Fixes
Tests
onOpenChangecallback is triggered when the "Open" button is clicked.