fix(show-confirm-dialog): eliminate react-dom/client build error on R…#216
Merged
fix(show-confirm-dialog): eliminate react-dom/client build error on R…#216
Conversation
…eact 16
The webpackIgnore magic comment on the dynamic import("react-dom/client")
was stripped from the compiled UMD output, causing consuming projects on
React 16 to fail with "Module not found: Can't resolve 'react-dom/client'".
Replace the version-detection approach with a bridge pattern:
- Add ConfirmDialogProvider that renders dialogs inside the existing React
tree, removing the need for createRoot or ReactDOM.render entirely
- showConfirmDialog() delegates to the provider when mounted
- Falls back to ReactDOM.render() with a console warning for apps that
haven't added the provider yet (React 16/17/18 backward compat)
- Zero references to react-dom/client remain in source or compiled output
smarcet
requested review from
santipalenque and
tomrndom
and removed request for
santipalenque
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRefactors confirm dialog from imperative DOM rendering to a declarative global-bridge pattern. Adds a exported Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant SCD as showConfirmDialog()
participant GB as globalThis Bridge
participant GCD as GlobalConfirmDialog
participant CD as ConfirmDialog
participant User as User
rect rgba(100, 150, 200, 0.5)
Note over GCD: Component mounts
GCD->>GB: registerBridge(callback)
end
rect rgba(100, 200, 150, 0.5)
App->>SCD: call showConfirmDialog(options)
SCD->>GB: checkBridge()
alt bridge registered
SCD->>GB: invoke callback(options, resolve)
GB->>GCD: set dialogState(options, resolve)
GCD->>CD: render ConfirmDialog with state
CD->>User: display dialog
User->>CD: click confirm/cancel
CD->>GCD: call onResolve(result)
GCD->>GCD: resolve promise, clear state
else bridge missing
SCD->>SCD: throw Error("GlobalConfirmDialog not mounted")
end
end
rect rgba(200, 150, 100, 0.5)
Note over GCD: Component unmounts
GCD->>GB: unregisterBridge()
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ 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 |
![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
🧹 Nitpick comments (2)
src/components/mui/__tests__/confirm-dialog-provider.test.js (1)
54-60: Consider usingjest.useFakeTimers()orwaitForfor more deterministic timing.The 10ms
setTimeoutto verify the promise hasn't resolved is fragile and could lead to flaky tests on slow CI environments. However, this is a minor concern given the simple use case.♻️ Alternative approach using fake timers
// Promise should not resolve yet let resolved = false; promise.then(() => { resolved = true; }); - await new Promise((r) => setTimeout(r, 10)); + // Flush pending microtasks without advancing time + await Promise.resolve(); expect(resolved).toBe(false);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/mui/__tests__/confirm-dialog-provider.test.js` around lines 54 - 60, The test currently uses a fragile real 10ms setTimeout to assert a promise hasn't resolved (see the local promise variable and resolved flag in confirm-dialog-provider.test.js); switch to deterministic timing by enabling fake timers (call jest.useFakeTimers() at the start of the test or describe block), then after attaching promise.then(()=> resolved = true) advance timers with jest.advanceTimersByTime(10) (or runAllTimers()) and assert expect(resolved).toBe(false); alternatively import waitFor from `@testing-library/react` and replace the setTimeout/expect block with await waitFor(() => expect(resolved).toBe(false), { timeout: 0 }) to avoid real timers—modify the test setup/teardown to restore timers with jest.useRealTimers() if you use fake timers.src/components/mui/__tests__/show-confirm-dialog.test.js (1)
40-45: Test relies on implementation details; consider verifying imports instead oftoString().Using
showConfirmDialog.toString()checks the transpiled function body, which is fragile across different build configurations. While checking for the absence of a string is more resilient than checking for presence, a more robust approach would verify the actual imports (e.g., viarequire.cacheinspection or static module analysis) rather than string matching on function output.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/mui/__tests__/show-confirm-dialog.test.js` around lines 40 - 45, The test currently inspects showConfirmDialog.toString(), which is brittle; change it to assert actual module imports instead: in the test for showConfirmDialog, use Jest's module isolation (e.g., jest.isolateModules or require.cache inspection) to require the module afresh and verify that "react-dom/client" is not present in the resolved module list (or require.cache) for that import; locate the test referencing showConfirmDialog in show-confirm-dialog.test.js and replace the toString() string check with an import-time inspection that confirms "react-dom/client" was not loaded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/mui/ConfirmDialogProvider.js`:
- Around line 35-42: bridgeCallback currently overwrites dialog state (including
onResolve) so concurrent calls orphan the first promise; before calling
setDialogState in bridgeCallback, detect if a dialog is already open (inspect
dialogState.open or keep a currentRef of onResolve) and auto-cancel the existing
dialog by invoking its onResolve (e.g., resolve with a cancel value or reject)
and clearing it, then proceed to setDialogState for the new dialog; update usage
of setDialogState/bridgeCallback to ensure you call the previous onResolve (from
dialogState or a stored ref) safely before replacing it so no promise is left
unresolved.
---
Nitpick comments:
In `@src/components/mui/__tests__/confirm-dialog-provider.test.js`:
- Around line 54-60: The test currently uses a fragile real 10ms setTimeout to
assert a promise hasn't resolved (see the local promise variable and resolved
flag in confirm-dialog-provider.test.js); switch to deterministic timing by
enabling fake timers (call jest.useFakeTimers() at the start of the test or
describe block), then after attaching promise.then(()=> resolved = true) advance
timers with jest.advanceTimersByTime(10) (or runAllTimers()) and assert
expect(resolved).toBe(false); alternatively import waitFor from
`@testing-library/react` and replace the setTimeout/expect block with await
waitFor(() => expect(resolved).toBe(false), { timeout: 0 }) to avoid real
timers—modify the test setup/teardown to restore timers with
jest.useRealTimers() if you use fake timers.
In `@src/components/mui/__tests__/show-confirm-dialog.test.js`:
- Around line 40-45: The test currently inspects showConfirmDialog.toString(),
which is brittle; change it to assert actual module imports instead: in the test
for showConfirmDialog, use Jest's module isolation (e.g., jest.isolateModules or
require.cache inspection) to require the module afresh and verify that
"react-dom/client" is not present in the resolved module list (or require.cache)
for that import; locate the test referencing showConfirmDialog in
show-confirm-dialog.test.js and replace the toString() string check with an
import-time inspection that confirms "react-dom/client" was not loaded.
🪄 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: 88340a5e-bbf1-4941-bc6c-69c900d26ca6
📒 Files selected for processing (6)
src/components/index.jssrc/components/mui/ConfirmDialogProvider.jssrc/components/mui/__tests__/confirm-dialog-provider.test.jssrc/components/mui/__tests__/show-confirm-dialog.test.jssrc/components/mui/showConfirmDialog.jswebpack.common.js
Comment on lines
+35
to
+42
| const bridgeCallback = (options) => { | ||
| return new Promise((resolve) => { | ||
| setDialogState({ | ||
| ...options, | ||
| open: true, | ||
| onResolve: resolve | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Overlapping dialog calls will orphan the first dialog's promise.
If showConfirmDialog is called while a dialog is already open, setDialogState overwrites the existing state including onResolve, causing the first caller's promise to never resolve.
This may be acceptable if overlapping calls are considered a usage error, but consider either:
- Queuing dialogs
- Rejecting/resolving the existing dialog before showing a new one
- Documenting this as expected behavior
🛡️ Option: Auto-cancel existing dialog before showing new one
useEffect(() => {
// Register the bridge callback when the provider mounts
const bridgeCallback = (options) => {
return new Promise((resolve) => {
+ // Cancel any existing dialog
+ setDialogState((prev) => {
+ if (prev?.onResolve) {
+ prev.onResolve(false);
+ }
+ return {
+ ...options,
+ open: true,
+ onResolve: resolve
+ };
+ });
- setDialogState({
- ...options,
- open: true,
- onResolve: resolve
- });
});
};📝 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.
Suggested change
| const bridgeCallback = (options) => { | |
| return new Promise((resolve) => { | |
| setDialogState({ | |
| ...options, | |
| open: true, | |
| onResolve: resolve | |
| }); | |
| }); | |
| const bridgeCallback = (options) => { | |
| return new Promise((resolve) => { | |
| // Cancel any existing dialog | |
| setDialogState((prev) => { | |
| if (prev?.onResolve) { | |
| prev.onResolve(false); | |
| } | |
| return { | |
| ...options, | |
| open: true, | |
| onResolve: resolve | |
| }; | |
| }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/mui/ConfirmDialogProvider.js` around lines 35 - 42,
bridgeCallback currently overwrites dialog state (including onResolve) so
concurrent calls orphan the first promise; before calling setDialogState in
bridgeCallback, detect if a dialog is already open (inspect dialogState.open or
keep a currentRef of onResolve) and auto-cancel the existing dialog by invoking
its onResolve (e.g., resolve with a cancel value or reject) and clearing it,
then proceed to setDialogState for the new dialog; update usage of
setDialogState/bridgeCallback to ensure you call the previous onResolve (from
dialogState or a stored ref) safely before replacing it so no promise is left
unresolved.
Contributor
santipalenque
left a comment
santipalenque
left a comment
There was a problem hiding this comment.
@smarcet this works but I think we overthinking this solution, there is no reason why we can just put the GlobalDialog component in so it renders always, and trigger the show/hide with a context solution. I think there is no good reason to spawn the dialog component and kill it every time we use it
…eact 16 compat Replace ReactDOM.render fallback with bridge pattern. GlobalConfirmDialog component and showConfirmDialog function live in a single module. showConfirmDialog requires <GlobalConfirmDialog /> at the app root — throws a clear error otherwise. No react-dom imports remain. BREAKING CHANGE: <GlobalConfirmDialog /> must be rendered at the app root.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/mui/showConfirmDialog.js`:
- Around line 95-113: The current bridgeFn implementation can orphan an
in-flight showConfirmDialog promise when dialogState is replaced or the
component unmounts; modify the useEffect and bridgeFn so that before setting a
new dialogState you check for an existing dialogState.onResolve and call it with
false (or reject) to settle it, and also in the cleanup returned by useEffect
call dialogState?.onResolve(false) (or reject) before nulling bridgeFn; update
references in bridgeFn, setDialogState, handleConfirm and handleCancel to ensure
any existing onResolve is invoked exactly once before replacing or clearing
dialogState.
🪄 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: 257e706f-7e66-455d-9ace-2f9524706b5f
📒 Files selected for processing (6)
package.jsonsrc/components/index.jssrc/components/mui/__tests__/global-confirm-dialog.test.jssrc/components/mui/__tests__/show-confirm-dialog.test.jssrc/components/mui/showConfirmDialog.jswebpack.common.js
🚧 Files skipped from review as they are similar to previous changes (3)
- package.json
- webpack.common.js
- src/components/mui/tests/show-confirm-dialog.test.js
Comment on lines
+95
to
+113
| const [dialogState, setDialogState] = useState(null); | ||
|
|
||
| useEffect(() => { | ||
| bridgeFn = (options) => { | ||
| return new Promise((resolve) => { | ||
| setDialogState({ ...options, open: true, onResolve: resolve }); | ||
| }); | ||
| }; | ||
| return () => { bridgeFn = null; }; | ||
| }, []); | ||
|
|
||
| const handleConfirm = () => close(true); | ||
| const handleCancel = () => close(false); | ||
| const handleConfirm = () => { | ||
| if (dialogState?.onResolve) dialogState.onResolve(true); | ||
| setDialogState(null); | ||
| }; | ||
|
|
||
| const element = ( | ||
| <ConfirmDialog | ||
| open | ||
| title={title} | ||
| text={text} | ||
| iconType={iconType} | ||
| confirmButtonText={confirmButtonText} | ||
| cancelButtonText={cancelButtonText} | ||
| confirmButtonColor={confirmButtonColor} | ||
| cancelButtonColor={cancelButtonColor} | ||
| onConfirm={handleConfirm} | ||
| onCancel={handleCancel} | ||
| /> | ||
| ); | ||
| const handleCancel = () => { | ||
| if (dialogState?.onResolve) dialogState.onResolve(false); | ||
| setDialogState(null); |
There was a problem hiding this comment.
Settle or reject the active promise before replacing/tearing down the dialog.
An in-flight showConfirmDialog() promise can be orphaned here. A second call overwrites dialogState.onResolve, and unmount cleanup only clears bridgeFn, so earlier callers can await forever. Please either queue dialogs or explicitly reject/resolve the active one before replacing it and during unmount.
Possible fix
-import React, { useState, useEffect } from "react";
+import React, { useState, useEffect, useRef } from "react";
@@
export const GlobalConfirmDialog = () => {
const [dialogState, setDialogState] = useState(null);
+ const resolverRef = useRef(null);
useEffect(() => {
bridgeFn = (options) => {
+ if (resolverRef.current) {
+ return Promise.reject(
+ new Error("[openstack-uicore-foundation] showConfirmDialog: another dialog is already active.")
+ );
+ }
+
return new Promise((resolve) => {
- setDialogState({ ...options, open: true, onResolve: resolve });
+ resolverRef.current = resolve;
+ setDialogState({ ...options, open: true });
});
};
- return () => { bridgeFn = null; };
+ return () => {
+ resolverRef.current?.(false);
+ resolverRef.current = null;
+ bridgeFn = null;
+ };
}, []);
const handleConfirm = () => {
- if (dialogState?.onResolve) dialogState.onResolve(true);
+ resolverRef.current?.(true);
+ resolverRef.current = null;
setDialogState(null);
};
const handleCancel = () => {
- if (dialogState?.onResolve) dialogState.onResolve(false);
+ resolverRef.current?.(false);
+ resolverRef.current = null;
setDialogState(null);
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/mui/showConfirmDialog.js` around lines 95 - 113, The current
bridgeFn implementation can orphan an in-flight showConfirmDialog promise when
dialogState is replaced or the component unmounts; modify the useEffect and
bridgeFn so that before setting a new dialogState you check for an existing
dialogState.onResolve and call it with false (or reject) to settle it, and also
in the cleanup returned by useEffect call dialogState?.onResolve(false) (or
reject) before nulling bridgeFn; update references in bridgeFn, setDialogState,
handleConfirm and handleCancel to ensure any existing onResolve is invoked
exactly once before replacing or clearing dialogState.
santipalenque
approved these changes
Apr 10, 2026
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/components/mui/showConfirmDialog.js (1)
102-109:⚠️ Potential issue | 🟠 MajorSettle active promise before replacement/unmount to avoid hanging callers.
Line 103 replaces the active resolver on concurrent calls, and Line 108 unmount cleanup drops the bridge without resolving the in-flight promise. That can leave earlier
showConfirmDialog()callers pending indefinitely.Proposed fix
-import React, { useState, useEffect } from "react"; +import React, { useState, useEffect, useRef } from "react"; @@ export const GlobalConfirmDialog = () => { const [dialogState, setDialogState] = useState(null); + const resolverRef = useRef(null); useEffect(() => { _global[BRIDGE_KEY] = (options) => { + if (resolverRef.current) { + // settle previous caller before replacing dialog + resolverRef.current(false); + resolverRef.current = null; + } return new Promise((resolve) => { - setDialogState({ ...options, open: true, onResolve: resolve }); + resolverRef.current = resolve; + setDialogState({ ...options, open: true }); }); }; - return () => { _global[BRIDGE_KEY] = null; }; + return () => { + resolverRef.current?.(false); + resolverRef.current = null; + _global[BRIDGE_KEY] = null; + }; }, []); const handleConfirm = () => { - if (dialogState?.onResolve) dialogState.onResolve(true); + resolverRef.current?.(true); + resolverRef.current = null; setDialogState(null); }; const handleCancel = () => { - if (dialogState?.onResolve) dialogState.onResolve(false); + resolverRef.current?.(false); + resolverRef.current = null; setDialogState(null); };Also applies to: 111-119
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/mui/showConfirmDialog.js` around lines 102 - 109, The bridge replacement in useEffect overwrites any existing resolver and the cleanup removes the bridge without settling an in-flight promise, which can leave callers of showConfirmDialog() hanging; modify the useEffect that assigns _global[BRIDGE_KEY] so it first checks for an existing pending resolver (store it in a local variable like prevResolve when setting _global), call that resolver (e.g. resolve with a canceled value or reject) before replacing it, and in the cleanup also detect and call any remaining onResolve to settle the promise; update references to _global[BRIDGE_KEY], setDialogState, and onResolve accordingly so no pending promise is left unresolved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/components/mui/showConfirmDialog.js`:
- Around line 102-109: The bridge replacement in useEffect overwrites any
existing resolver and the cleanup removes the bridge without settling an
in-flight promise, which can leave callers of showConfirmDialog() hanging;
modify the useEffect that assigns _global[BRIDGE_KEY] so it first checks for an
existing pending resolver (store it in a local variable like prevResolve when
setting _global), call that resolver (e.g. resolve with a canceled value or
reject) before replacing it, and in the cleanup also detect and call any
remaining onResolve to settle the promise; update references to
_global[BRIDGE_KEY], setDialogState, and onResolve accordingly so no pending
promise is left unresolved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 407bcd78-c265-48ea-8218-f065d600426a
📒 Files selected for processing (2)
package.jsonsrc/components/mui/showConfirmDialog.js
✅ Files skipped from review due to trivial changes (1)
- package.json
tomrndom
approved these changes
Apr 10, 2026
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.
ref: https://app.clickup.com/t/86b94xa4t
Summary by CodeRabbit
New Features
Behavior
Tests
Chores