fix(angular/modal): Fix programmatic modal closing#2621
Conversation
✅ Deploy Preview for ix-storybook canceled.
|
📝 WalkthroughWalkthrough
ChangesPending Modal Action Queue
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
🦋 Changeset detectedLatest commit: afd94e0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to queue modal actions ('close' or 'dismiss') when the underlying modal element is not yet initialized, preventing errors during programmatic modal closing. It also adds comprehensive unit tests to verify this behavior. The review feedback suggests adding a changeset file for this consumer-relevant bug fix as required by the style guide, and recommends refactoring the conditional logic in 'setModalElement' to use an 'else if' block instead of an early return.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
@dmytro-halimov Just an idea my side would it make sense to have some QoL feature like a |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/angular/common/src/providers/modal/modal-ref.ts (1)
37-61: 📐 Maintainability & Code Quality | 🟡 MinorAdd an
@siemens/ix-angularchangeset for this modal behavior change.
close/dismissnow defer untilmodalElementexists, so this is consumer-visible and needs a changeset unless it is strictly internal-only. IncludeFixes#2595`` in the summary if this release should close the issue.🤖 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 `@packages/angular/common/src/providers/modal/modal-ref.ts` around lines 37 - 61, The deferred close/dismiss behavior in modal-ref is consumer-visible, so add a changeset for this change in `@siemens/ix-angular`. Update the release note summary to describe that ModalRef.close and ModalRef.dismiss now queue a pendingAction until modalElement exists, and include Fixes `#2595` in the summary if this release should close the issue. Use the ModalRef class and its close/dismiss behavior as the reference point when creating the changeset.Source: Path instructions
🤖 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.
Outside diff comments:
In `@packages/angular/common/src/providers/modal/modal-ref.ts`:
- Around line 37-61: The deferred close/dismiss behavior in modal-ref is
consumer-visible, so add a changeset for this change in `@siemens/ix-angular`.
Update the release note summary to describe that ModalRef.close and
ModalRef.dismiss now queue a pendingAction until modalElement exists, and
include Fixes `#2595` in the summary if this release should close the issue. Use
the ModalRef class and its close/dismiss behavior as the reference point when
creating the changeset.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2950e442-5c80-4a2e-963c-fe3d39ee0b78
📒 Files selected for processing (2)
packages/angular/common/src/providers/modal/modal-ref.tspackages/angular/common/src/providers/modal/modal.service.spec.ts
danielleroux
left a comment
danielleroux
left a comment
There was a problem hiding this comment.
- Changeset missing
- PR Title is misleading change is not core related only angular related
- Adapt description to make clear that only the angular service is affected
dmytro-halimov
changed the title
yeah I think this would make sense for core package. But if I get it correctly, these data attributes would only handle user clicks after initial render happens, so it wouldn't cover these programmatic race conditions, the queue in Angular package is still needed. |
|
3 participants
GitHub Issue Number: #2595
JIRA Ref: #IX-4365
💡 What is the new behavior?
[Angular package]: Fixes modal closing race condition where
close()ordismiss()called before modal element initialization would silently fail. Pending actions are now queued and executed once the modal element is set.🏁 Checklist
A pull request can only be merged if all of these conditions are met (where applicable):
pnpm test)pnpm lint)pnpm build, changes pushed)👨💻 Help & support
Summary by CodeRabbit
close()/dismiss()could be missed if called before the modal was fully initialized.closeanddismissare requested early, only the first pending action is applied to keep behavior consistent.