feat: allow to stack modals on top of each other#3203
Merged
Conversation
|
Size Change: +392 B (+0.06%) Total Size: 656 kB 📦 View Changed
ℹ️ View Unchanged
|
oliverlaz
approved these changes
Jun 3, 2026
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds ordered opened-dialog tracking and topmost selectors; scopes GlobalModal by per-instance dialog id and gates close/overlay/Escape to the topmost dialog; updates tests and accessibility wiring to validate opened-id order, topmost isolation, and derived aria ids. ChangesStacked Modal & Dialog Topmost State
Sequence DiagramsequenceDiagram
participant User
participant ChildModal as GlobalModal(child)
participant DialogMgr as DialogManager
participant ParentModal as GlobalModal(parent)
User->>ChildModal: overlay / Escape
ChildModal->>DialogMgr: isTopmost?(childId)
DialogMgr-->>ChildModal: true/false (based on openedDialogIds)
ChildModal->>DialogMgr: close(childId) (when topmost)
DialogMgr->>DialogMgr: remove childId from openedDialogIds
DialogMgr-->>ParentModal: isTopmost?(parentId)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3203 +/- ##
==========================================
+ Coverage 83.81% 83.85% +0.04%
==========================================
Files 439 439
Lines 13192 13203 +11
Branches 4274 4280 +6
==========================================
+ Hits 11057 11072 +15
+ Misses 2135 2131 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/Modal/GlobalModal.tsx (1)
152-165:⚠️ Potential issue | 🟠 Major | ⚖️ Poor tradeoffGate non-topmost modal from the accessibility tree when stacking dialogs
FocusScope’scontain={isTopmost}traps keyboard focus, but it doesn’t hide the non-topmost dialog content from screen readers.GlobalModalalways renders the dialog surface witharia-modal='true'(aroundsrc/components/Modal/GlobalModal.tsx:152-165) even when!isTopmost, and there’s no existinginert/aria-hiddenhandling for stacked modals elsewhere in the dialog/modal implementation.- When
!isTopmost, applyinert(oraria-hidden) to the background modal layer/surface and ensure only the topmost modal has modal semantics (e.g., remove/disablearia-modalon background dialogs).🤖 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/GlobalModal.tsx` around lines 152 - 165, GlobalModal currently renders every stacked dialog surface with aria-modal="true" and only uses FocusScope(contain={isTopmost}) to trap focus; to fix, when rendering the dialog div inside FocusScope (symbols: FocusScope, isTopmost, GlobalModal, handleDialogKeyDown, role), ensure only the topmost modal has aria-modal and that non-topmost dialogs are gated from assistive tech by adding inert or aria-hidden (e.g., set aria-modal={isTopmost ? 'true' : undefined} and add inert or aria-hidden={isTopmost ? undefined : true} to the dialog container); keep existing keyboard handlers and tabIndex behavior unchanged so only the topmost modal retains modal semantics and screen-reader visibility.
🧹 Nitpick comments (1)
src/components/Modal/__tests__/GlobalModal.test.tsx (1)
19-42: 💤 Low valueNit:
renderStackedModalsreferencesModalContentbefore its declaration (Line 45).It's safe in practice because the helper body only executes inside
it(...)callbacks (after module evaluation), so no temporal-dead-zone error occurs. MovingModalContentabove this helper would make the ordering self-evident.🤖 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/__tests__/GlobalModal.test.tsx` around lines 19 - 42, renderStackedModals references ModalContent before its declaration; move the ModalContent component declaration above renderStackedModals so the helper doesn't rely on a later-declared symbol. Update the test file so ModalContent is declared prior to the renderStackedModals function (referencing ModalContent and renderStackedModals) to make the dependency order explicit and avoid potential TDZ confusion.
🤖 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/Message/__tests__/MessageText.test.tsx`:
- Around line 418-445: The tests in MessageText.test.tsx ("should render with a
custom wrapper class when one is set", "should render with a custom inner class
when one is set", and "should render with custom theme identifier in generated
css classes when theme is set") only assert visible text and thus don't verify
the actual class/theme behavior; update each test that calls renderMessageText
(with customProps: { customWrapperClass }, { customInnerClass }, and { theme:
'custom' }) to assert the DOM contains the expected class names or theme-based
CSS selectors (e.g., query the element returned by renderMessageText/getByText
and check classList contains the custom class or that a generated class includes
the "custom" identifier) or replace the assertion with a targeted snapshot
asserting those class selectors; use the existing helper renderMessageText and
generated message from generateMessage to locate the element to assert against.
---
Outside diff comments:
In `@src/components/Modal/GlobalModal.tsx`:
- Around line 152-165: GlobalModal currently renders every stacked dialog
surface with aria-modal="true" and only uses FocusScope(contain={isTopmost}) to
trap focus; to fix, when rendering the dialog div inside FocusScope (symbols:
FocusScope, isTopmost, GlobalModal, handleDialogKeyDown, role), ensure only the
topmost modal has aria-modal and that non-topmost dialogs are gated from
assistive tech by adding inert or aria-hidden (e.g., set aria-modal={isTopmost ?
'true' : undefined} and add inert or aria-hidden={isTopmost ? undefined : true}
to the dialog container); keep existing keyboard handlers and tabIndex behavior
unchanged so only the topmost modal retains modal semantics and screen-reader
visibility.
---
Nitpick comments:
In `@src/components/Modal/__tests__/GlobalModal.test.tsx`:
- Around line 19-42: renderStackedModals references ModalContent before its
declaration; move the ModalContent component declaration above
renderStackedModals so the helper doesn't rely on a later-declared symbol.
Update the test file so ModalContent is declared prior to the
renderStackedModals function (referencing ModalContent and renderStackedModals)
to make the dependency order explicit and avoid potential TDZ confusion.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 566f0b54-5638-45b8-b0fb-4608b7b75e1f
⛔ Files ignored due to path filters (1)
src/components/Message/__tests__/__snapshots__/MessageText.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (10)
src/components/Dialog/__tests__/DialogManagerContext.test.tsxsrc/components/Dialog/__tests__/DialogsManager.test.tssrc/components/Dialog/hooks/useDialog.tssrc/components/Dialog/service/DialogManager.tssrc/components/Dialog/styling/Alert.scsssrc/components/Message/__tests__/MessageText.test.tsxsrc/components/MessageActions/__tests__/MessageActions.test.tsxsrc/components/MessageComposer/__tests__/AttachmentSelector.test.tsxsrc/components/Modal/GlobalModal.tsxsrc/components/Modal/__tests__/GlobalModal.test.tsx
# Conflicts: # src/components/Modal/__tests__/GlobalModal.test.tsx
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/components/Message/__tests__/MessageText.test.tsx (2)
422-432:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix: Pass
customWrapperClassdirectly incustomProps, not wrapped in aMessagecomponent.The current approach passes a
Messagecomponent function incustomProps, but this component is never invoked—it's just passed as an unused prop to the<Message>component (line 122 inrenderMessageText). As a result,customWrapperClassnever reaches the<MessageText>component (line 123), and the assertion will fail.The
renderMessageTexthelper spreadscustomPropsinto both<Message>and<MessageText>:<Message {...customProps}> <MessageText {...customProps} /> </Message>So to pass
customWrapperClasstoMessageText, include it directly incustomProps.🔧 Proposed fix
it('should render with a custom wrapper class when one is set', async () => { const customWrapperClass = 'custom-wrapper'; const message = generateMessage({ text: 'hello world' }); const { getByText } = await renderMessageText({ customProps: { + customWrapperClass, message, - Message: () => ( - <MessageText customWrapperClass={customWrapperClass} message={message} /> - ), }, }); expect(getByText('hello world').closest(`.${customWrapperClass}`)).toHaveClass( customWrapperClass, ); });🤖 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/Message/__tests__/MessageText.test.tsx` around lines 422 - 432, The test is passing customWrapperClass inside a fake Message component so it never reaches MessageText; update the test to include customWrapperClass directly in the customProps object passed to renderMessageText (so that renderMessageText spreads it into both <Message> and <MessageText>), i.e., remove the Message: () => (...) wrapper and put customWrapperClass: customWrapperClass at the top level of customProps so MessageText receives it and the assertion can find the wrapper class.
438-447:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix: Pass
customInnerClassdirectly incustomProps, not wrapped in aMessagecomponent.Same issue as the previous test—the
Messagecomponent incustomPropsis never invoked, socustomInnerClassdoesn't reach<MessageText>.🔧 Proposed fix
it('should render with a custom inner class when one is set', async () => { const customInnerClass = 'custom-inner'; const message = generateMessage({ text: 'hi mate' }); const { getByTestId } = await renderMessageText({ customProps: { + customInnerClass, message, - Message: () => ( - <MessageText customInnerClass={customInnerClass} message={message} /> - ), }, }); expect(getByTestId(messageTextTestId)).toHaveClass(customInnerClass); });🤖 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/Message/__tests__/MessageText.test.tsx` around lines 438 - 447, The test is passing customInnerClass incorrectly by wrapping MessageText in a Message component so it never gets invoked; change the call to renderMessageText to include customInnerClass directly in customProps (e.g. customProps: { message, customInnerClass, MessageText } or simply customProps: { message, customInnerClass } depending on renderMessageText signature) so that MessageText receives the prop, then assert with getByTestId(messageTextTestId) toHaveClass(customInnerClass); update references to renderMessageText, customProps, Message, MessageText, customInnerClass and messageTextTestId accordingly.
🤖 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.
Duplicate comments:
In `@src/components/Message/__tests__/MessageText.test.tsx`:
- Around line 422-432: The test is passing customWrapperClass inside a fake
Message component so it never reaches MessageText; update the test to include
customWrapperClass directly in the customProps object passed to
renderMessageText (so that renderMessageText spreads it into both <Message> and
<MessageText>), i.e., remove the Message: () => (...) wrapper and put
customWrapperClass: customWrapperClass at the top level of customProps so
MessageText receives it and the assertion can find the wrapper class.
- Around line 438-447: The test is passing customInnerClass incorrectly by
wrapping MessageText in a Message component so it never gets invoked; change the
call to renderMessageText to include customInnerClass directly in customProps
(e.g. customProps: { message, customInnerClass, MessageText } or simply
customProps: { message, customInnerClass } depending on renderMessageText
signature) so that MessageText receives the prop, then assert with
getByTestId(messageTextTestId) toHaveClass(customInnerClass); update references
to renderMessageText, customProps, Message, MessageText, customInnerClass and
messageTextTestId accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4e76d3be-e466-409f-94b5-ce0c5e9c4b97
📒 Files selected for processing (3)
src/components/Message/__tests__/MessageText.test.tsxsrc/components/Modal/GlobalModal.tsxsrc/components/Modal/__tests__/GlobalModal.test.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/Modal/tests/GlobalModal.test.tsx
- src/components/Modal/GlobalModal.tsx
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
2 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.
🎯 Goal
The SDK needs to be able to display modals above each other with new ChannelDetails component. ChannelDetails is rendered in modal and contains buttons that invoke confirmation dialogs on top, e.g. "Delete chat". This PR adds possibility to have multiple open dialogs / modals at the same time.
Summary by CodeRabbit
New Features
Bug Fixes
Tests