fix: improve connection handling for external modals #7035
Conversation
- Added logic to manage the connection state between the modal and its parent component. - Introduced a flag to track if the modal is connected, ensuring proper handling during re-creation and API extension. - Enhanced error handling when connecting to the parent modal's API.
MiniJude
requested review from
anncwb,
jinmao88,
mynetfan and
vince292007
as code owners
🦋 Changeset detectedLatest commit: 6c128cc The changes in this PR will be included in the next version bump. This PR includes changesets to release 43 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 |
WalkthroughIntroduces a dedicated Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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 (1)
packages/@core/ui-kit/popup-ui/src/modal/use-modal.ts (1)
79-82: Consider improving type safety.The
injectedvariable is typed asany, which bypasses TypeScript's type checking. While this may be intentional for flexibility, consider defining an interface for the injection context to improve type safety and IDE support.🔎 Suggested improvement
Define an interface for the injection context:
interface ModalInjectContext { __vbenModalConnected: boolean; extendApi?: (api: ExtendedModalApi) => void; options?: ModalApiOptions; reCreateModal?: () => Promise<void>; }Then update line 79:
- const injected = inject<any>(USER_MODAL_INJECT_KEY, {}); + const injected = inject<ModalInjectContext>(USER_MODAL_INJECT_KEY, {});
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/@core/ui-kit/popup-ui/src/modal/use-modal.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: mynetfan
Repo: vbenjs/vue-vben-admin PR: 5397
File: playground/src/bootstrap.ts:23-30
Timestamp: 2025-01-15T04:29:13.944Z
Learning: In the Vue-Vben-Admin project, commented code in bootstrap.ts showing modal and drawer configuration examples should be preserved as reference documentation, even after moving to CSS variables for z-index management.
🧬 Code graph analysis (1)
packages/@core/ui-kit/popup-ui/src/modal/use-modal.ts (1)
packages/@core/ui-kit/popup-ui/src/modal/modal.ts (1)
ExtendedModalApi(154-158)
⏰ 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: post-update (ubuntu-latest)
- GitHub Check: post-update (windows-latest)
🔇 Additional comments (1)
packages/@core/ui-kit/popup-ui/src/modal/use-modal.ts (1)
130-140: Try-catch structure is well-designed.The connection logic is properly wrapped in a try-catch block that:
- Sets the connection flag only after
extendApisucceeds (line 134)- Resets the flag on failure (line 137)
- Propagates errors to the caller (line 138)
Note: Line 137 is currently necessary because line 40 prematurely sets the flag to
true. Once line 40 is removed (as noted in the previous comment), line 137 becomes redundant but serves as defensive coding and can be kept.
| const injectContext: any = { | ||
| __vbenModalConnected: false, | ||
| extendApi(api: ExtendedModalApi) { | ||
| injectContext.__vbenModalConnected = true; | ||
| // 不能直接给 reactive 赋值，会丢失响应 | ||
| // 不能用 Object.assign,会丢失 api 的原型函数 | ||
| Object.setPrototypeOf(extendedApi, api); | ||
| }, | ||
| options, | ||
| async reCreateModal() { | ||
| injectContext.__vbenModalConnected = false; | ||
| isModalReady.value = false; | ||
| await nextTick(); | ||
| isModalReady.value = true; | ||
| }, | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Line 40 should be removed to complete the fix.
According to the PR objectives, the fix should "move setting of __vbenModalConnected to after extendApi succeeds." However, line 40 still sets the flag prematurely before Object.setPrototypeOf on line 43 completes. If setPrototypeOf throws, the flag will have been set to true temporarily.
While the catch block on lines 135-139 attempts to handle this by resetting the flag to false, the design is fragile. The correct fix is to remove line 40 entirely and rely solely on line 134 (which sets the flag after the entire extendApi call succeeds).
🔎 Proposed fix
const injectContext: any = {
__vbenModalConnected: false,
extendApi(api: ExtendedModalApi) {
- injectContext.__vbenModalConnected = true;
// 不能直接给 reactive 赋值，会丢失响应
// 不能用 Object.assign,会丢失 api 的原型函数
Object.setPrototypeOf(extendedApi, api);
},This ensures the flag is set only once, at the correct location (line 134), after confirming the entire connection succeeded.
📝 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 injectContext: any = { | |
| __vbenModalConnected: false, | |
| extendApi(api: ExtendedModalApi) { | |
| injectContext.__vbenModalConnected = true; | |
| // 不能直接给 reactive 赋值，会丢失响应 | |
| // 不能用 Object.assign,会丢失 api 的原型函数 | |
| Object.setPrototypeOf(extendedApi, api); | |
| }, | |
| options, | |
| async reCreateModal() { | |
| injectContext.__vbenModalConnected = false; | |
| isModalReady.value = false; | |
| await nextTick(); | |
| isModalReady.value = true; | |
| }, | |
| }); | |
| }; | |
| const injectContext: any = { | |
| __vbenModalConnected: false, | |
| extendApi(api: ExtendedModalApi) { | |
| // 不能直接给 reactive 赋值，会丢失响应 | |
| // 不能用 Object.assign,会丢失 api 的原型函数 | |
| Object.setPrototypeOf(extendedApi, api); | |
| }, | |
| options, | |
| async reCreateModal() { | |
| injectContext.__vbenModalConnected = false; | |
| isModalReady.value = false; | |
| await nextTick(); | |
| isModalReady.value = true; | |
| }, | |
| }; |
🤖 Prompt for AI Agents
In packages/@core/ui-kit/popup-ui/src/modal/use-modal.ts around lines 37 to 52,
remove the premature assignment that sets injectContext.__vbenModalConnected =
true at line 40; the flag should only be set after extendApi completes
successfully (as already done later around line 134), so delete that early set
to avoid transient true state if Object.setPrototypeOf throws and rely on the
existing post-success assignment.
|
刚刚复现你的问题，但未能复测出你的问题，drawer组件貌似有你说的 问题，Modal我没复测出来 |
您好，我复现了，可以clone一下代码本地启动：
|
|
有一个PR 修复了此问题 如果问题已解决 close一下这个pr |
3 participants
Added logic to manage the connection state between the modal and its parent component.
Introduced a flag to track if the modal is connected, ensuring proper handling
during re-creation and API extension.
Enhanced error handling when connecting to the parent modal's API.
Description
This PR fixes a bug in the modal connection handling logic where the
__vbenModalConnectedflag was set totruebefore verifying that the parent'sextendApicall succeeds. IfObject.setPrototypeOfwithinextendApithrows an error, the flag remainstruedespite the connection being incomplete. This prevents subsequent child modals from attempting to connect since they check this flag to decide whether to connect.The Problem:
本地复现代码：
git clone -b "pr-#7035问题复现" --single-branch https://github.com/MiniJude/vue-vben-admin.gitpnpm dev:playgroundLenovoPcManager_QDKPoDZvFi.mp4
injected.__vbenModalConnected = trueat line 131 before callingextendApiextendApithrows an error (e.g.,Object.setPrototypeOffails), the flag remainstrueThe Solution:
extendApicall succeedsfalseif connection fails, allowing retriesType of change
pnpm-lock.yamlunless you introduce a new test example.Checklist
pnpm run docs:devcommand.pnpm test.feat:,fix:,perf:,docs:, orchore:.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.