Enhance Modal component to support controlled and uncontrolled states with improved open state management #1609
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
WalkthroughThe Modal component now supports both controlled and uncontrolled open state via an Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
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}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/cyberstorm/src/newComponents/Modal/Modal.tsx(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build
- GitHub Check: Generate visual diffs
🔇 Additional comments (1)
packages/cyberstorm/src/newComponents/Modal/Modal.tsx (1)
210-220: LGTM—reasonable workaround for Radix layer stacking.The SSR guard and conditional restoration logic are sound. The custom event dispatch may help re-sync Radix's dismissable layer tracking.
| const handleOpenChange = useCallback( | ||
| (next: boolean) => { | ||
| onOpenChange?.(next); | ||
| if (!isControlled) { | ||
| setUncontrolledOpen(next); | ||
| } | ||
| if (!next) { | ||
| if (typeof window !== "undefined") { | ||
| window.requestAnimationFrame(restoreBodyPointerEvents); | ||
| } else { | ||
| restoreBodyPointerEvents(); | ||
| } | ||
| } | ||
| }, | ||
| [onOpenChange, isControlled, restoreBodyPointerEvents] | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Simplify after fixing state management.
Once internal state management is removed (per previous comment), lines 225-227 become unnecessary:
const handleOpenChange = useCallback(
(next: boolean) => {
onOpenChange?.(next);
- if (!isControlled) {
- setUncontrolledOpen(next);
- }
if (!next) {
if (typeof window !== "undefined") {
window.requestAnimationFrame(restoreBodyPointerEvents);
} else {
restoreBodyPointerEvents();
}
}
},
- [onOpenChange, isControlled, restoreBodyPointerEvents]
+ [onOpenChange, restoreBodyPointerEvents]
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleOpenChange = useCallback( | |
| (next: boolean) => { | |
| onOpenChange?.(next); | |
| if (!isControlled) { | |
| setUncontrolledOpen(next); | |
| } | |
| if (!next) { | |
| if (typeof window !== "undefined") { | |
| window.requestAnimationFrame(restoreBodyPointerEvents); | |
| } else { | |
| restoreBodyPointerEvents(); | |
| } | |
| } | |
| }, | |
| [onOpenChange, isControlled, restoreBodyPointerEvents] | |
| ); | |
| const handleOpenChange = useCallback( | |
| (next: boolean) => { | |
| onOpenChange?.(next); | |
| if (!next) { | |
| if (typeof window !== "undefined") { | |
| window.requestAnimationFrame(restoreBodyPointerEvents); | |
| } else { | |
| restoreBodyPointerEvents(); | |
| } | |
| } | |
| }, | |
| [onOpenChange, restoreBodyPointerEvents] | |
| ); |
🤖 Prompt for AI Agents
In packages/cyberstorm/src/newComponents/Modal/Modal.tsx around lines 222-237,
remove the now-unnecessary internal state update block (the if (!isControlled) {
setUncontrolledOpen(next); } lines) as internal state management was removed;
then update the handleOpenChange dependencies to drop isControlled (and any
setter) so the useCallback deps include only onOpenChange and
restoreBodyPointerEvents, leaving the function to call onOpenChange and handle
restoreBodyPointerEvents when closing.
| useEffect(() => { | ||
| if (!open) { | ||
| restoreBodyPointerEvents(); | ||
| } | ||
| return () => { | ||
| restoreBodyPointerEvents(); | ||
| }; | ||
| }, [open, restoreBodyPointerEvents]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Redundant restoration on close.
When the modal closes, handleOpenChange already calls restoreBodyPointerEvents (line 230), then this effect fires again because open changed. The unmount cleanup is valuable, but the !open branch duplicates work.
useEffect(() => {
- if (!open) {
- restoreBodyPointerEvents();
- }
return () => {
restoreBodyPointerEvents();
};
- }, [open, restoreBodyPointerEvents]);
+ }, [restoreBodyPointerEvents]);Now you only restore on unmount, avoiding the double-call when closing.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useEffect(() => { | |
| if (!open) { | |
| restoreBodyPointerEvents(); | |
| } | |
| return () => { | |
| restoreBodyPointerEvents(); | |
| }; | |
| }, [open, restoreBodyPointerEvents]); | |
| useEffect(() => { | |
| return () => { | |
| restoreBodyPointerEvents(); | |
| }; | |
| }, [restoreBodyPointerEvents]); |
🤖 Prompt for AI Agents
In packages/cyberstorm/src/newComponents/Modal/Modal.tsx around lines 239 to
246, the useEffect redundantly calls restoreBodyPointerEvents when open becomes
false (duplicate of the call in handleOpenChange at ~line 230); remove the if
(!open) branch so the effect only returns a cleanup that calls
restoreBodyPointerEvents on unmount, and update the effect dependency array to
only include restoreBodyPointerEvents to prevent it running on every open
change.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1609 +/- ##
=========================================
- Coverage 9.86% 9.84% -0.03%
=========================================
Files 309 309
Lines 22555 22601 +46
Branches 405 405
=========================================
Hits 2224 2224
- Misses 20331 20377 +46 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
… with improved open state management
Oksamies
force-pushed
the
11-13-enhance_modal_component_to_support_controlled_and_uncontrolled_states_with_improved_open_state_management
branch
from
ed8b2e0 to
c205045
Compare
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| {...(isControlled ? { open: controlledOpen } : {})} | ||
| {...(defaultOpen !== undefined ? { defaultOpen } : {})} |
There was a problem hiding this comment.
Bug: Passing both open and defaultOpen in controlled mode
The component passes defaultOpen to Dialog.Root even when in controlled mode. This violates React's controlled/uncontrolled component pattern - a component should never receive both open (controlled) and defaultOpen (uncontrolled) props simultaneously.
If a user provides both open={true} and defaultOpen={false}, both will be passed to Dialog.Root, causing unpredictable behavior.
Fix:
{...(isControlled ? { open: controlledOpen } : {})}
{...(!isControlled && defaultOpen !== undefined ? { defaultOpen } : {})}This ensures defaultOpen is only passed in uncontrolled mode.
| {...(isControlled ? { open: controlledOpen } : {})} | |
| {...(defaultOpen !== undefined ? { defaultOpen } : {})} | |
| {...(isControlled ? { open: controlledOpen } : {})} | |
| {...(!isControlled && defaultOpen !== undefined ? { defaultOpen } : {})} |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
No description provided.