fix(modal): fix issue #3450 #3650
Conversation
WalkthroughRender-time slot invocations in the Vue modal component are now wrapped as single-element arrays for default and footer slots; several Playwright test locators/assertions were adjusted to match resulting DOM index and scoping changes. No public API signature changes. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ModalComponent
participant SlotFunction as SlotFn
Note over ModalComponent,SlotFn #f0f8ff: Slot invocation shape normalized to array
User->>ModalComponent: render()
ModalComponent->>SlotFn: call(props, h)
SlotFn-->>ModalComponent: VNode (or array)
ModalComponent->>ModalComponent: ensure slot result is [VNode] (wrap if needed)
ModalComponent->>User: rendered VNode tree (slot content as array)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/vue/src/modal/src/pc.vue (1)
255-256: Optional: Normalize slot children for consistency and robustnessYour changes are correct and Vue will flatten nested arrays, but you can make slot handling more explicit by:
- Preventing nested arrays ([[…]])
- Filtering out
null/undefined/falsein one placeApply these diffs to both PC and mobile-first renders:
packages/vue/src/modal/src/pc.vue
- ? [defaultSlot.call(this, { $modal: this }, h)] + ? normalizeSlotChildren(defaultSlot.call(this, { $modal: this }, h)) … - ? [footerSlot.call(this, footerSlotParams, h)] + ? normalizeSlotChildren(footerSlot.call(this, footerSlotParams, h))packages/vue/src/modal/src/mobile-first.vue
- ? defaultSlot.call(this, { $modal: this }, h) + ? normalizeSlotChildren(defaultSlot.call(this, { $modal: this }, h)) … - footerSlot ? footerSlot.call(this, footerSlotParams, h) : footerBottom + footerSlot + ? normalizeSlotChildren(footerSlot.call(this, footerSlotParams, h)) + : footerBottomAdd a single helper at module scope (above your
renderfunction):// Normalize various slot return shapes into a flat children array const normalizeSlotChildren = (result: any): any[] => { if (result == null || result === false) return [] return Array.isArray(result) ? result : [result] }To catch any other slot-call sites that could benefit from this pattern:
rg -nP -C2 '\w+Slot\.call\(\s*this\s*,'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/vue/src/modal/src/pc.vue(2 hunks)
🔇 Additional comments (2)
packages/vue/src/modal/src/pc.vue (2)
254-261: LGTM: Array-wrapping the default slot result fixes Vue 2 children shape.This ensures the 3rd argument to h(...) is always an array, which aligns with Vue 2’s render expectations. Nested arrays will be normalized by Vue, so this is backward-safe across Vue 2/3. No public API change introduced.
292-320: LGTM: Footer slot return wrapped in an array for consistency and Vue 2 compatibility.Consistent with the body’s default slot handling and safe across Vue 2/3 due to children normalization.
James-9696
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
examples/sites/demos/pc/app/modal/modal-fn-slots.spec.ts (2)
9-10: Stabilize selectors by scoping to the visible modal instead of relying on nth(...) indicesIndex-based assertions are brittle and will break again if the DOM shifts (as happened due to slot wrapping). Scoping to the currently visible modal makes the test resilient without changing intent.
Apply this diff:
- await expect(content.nth(4)).toHaveText(/使用默认插槽/) - await expect(inner.nth(2)).toHaveText(/记录最近事项/) + // Scope to the visible modal to avoid index fragility + await expect( + page.locator('.tiny-modal').filter({ has: page.locator('.tiny-modal__box:visible') }).locator('.tiny-modal__content') + ).toHaveText(/使用默认插槽/) + await expect( + page.locator('.tiny-modal').filter({ has: page.locator('.tiny-modal__box:visible') }).getByRole('link', { name: /记录最近事项/ }) + ).toBeVisible()
11-11: Optionally assert the modal closes after clicking 取消A quick postcondition keeps regressions from slipping in when cancel behavior changes.
await page.getByRole('button', { name: '取消' }).click() + await expect(page.locator('.tiny-modal__box:visible')).toHaveCount(0)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
examples/sites/demos/pc/app/modal/modal-fn-slots.spec.ts(1 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). (1)
- GitHub Check: PR E2E Test (pnpm test:e2e3)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
examples/sites/demos/pc/app/modal/modal-header.spec.ts (1)
6-10: Scope assertions to the active dialog; avoid unnecessary first().Good move scoping the click to #modal-header. However, the header assertion still targets the whole page and the click uses first() unnecessarily. Prefer scoping the header to the active dialog to reduce cross-demo flakiness, and click the uniquely named button without first().
Apply this minimal diff:
- await demo.getByRole('button', { name: '自定义弹窗标题' }).first().click() + await demo.getByRole('button', { name: '自定义弹窗标题' }).click()Optionally tighten the header assertion (outside the selected lines):
// Scope to the active dialog to avoid matching stale titles. const dialog = page.getByRole('dialog').last() const header = dialog.locator('.tiny-modal__title') await expect(header).toHaveText(/自定义弹窗标题/)examples/sites/demos/pc/app/modal/basic-usage.spec.ts (1)
22-23: Replace broad div + nth filter with a robust text locator.Using page.locator('div').filter(...).nth(1) is brittle and order-dependent. Prefer Playwright’s text engine with last() and an inclusion assertion for stability.
- const info = page.locator('div') - await expect(info.filter({ hasText: '简单的消息' }).nth(1)).toHaveText(/简单的/) + const info = page.getByText('简单的消息', { exact: false }).last() + await expect(info).toBeVisible() + await expect(info).toContainText('简单的消息')examples/sites/demos/pc/app/modal/message-id.spec.ts (1)
7-8: Drop unnecessary await on locator and avoid nth-based assertions; also validate non-duplication.
- await on a Locator is a no-op and reads as a bug.
- div + nth(1) is order-sensitive; use a text locator and inclusive assertion.
- Given the test name, consider asserting the message does not duplicate after a second click.
Apply this diff:
- const info = await page.locator('div').filter({ hasText: '自定义消息具有唯一 id，所以不会重复出现' }) - await expect(info.nth(1)).toHaveText(/唯一 id/) + const info = page.getByText('自定义消息具有唯一 id，所以不会重复出现', { exact: false }).last() + await expect(info).toBeVisible() + await expect(info).toContainText('唯一 id')Optionally extend the test to verify uniqueness (outside the selected lines):
// Click again; still only one message should exist. await page.getByRole('button', { name: '重复点击不出现多个' }).click() await expect(page.getByText('自定义消息具有唯一 id，所以不会重复出现', { exact: false })).toHaveCount(1)examples/sites/demos/pc/app/modal/message-close.spec.ts (1)
7-8: Use a text locator instead of div + nth; optionally assert auto-close.Switch to a stable text locator and use inclusive text assertion. If feasible, add an auto-dismiss assertion to reflect “5s 后得自动关闭”.
- const info = page.locator('div').filter({ hasText: '自定义消息的内容可关闭，5s 后得自动关闭' }) - await expect(info.nth(1)).toHaveText(/5s 后得自动关闭/) + const info = page.getByText('自定义消息的内容可关闭，5s 后得自动关闭', { exact: false }).last() + await expect(info).toBeVisible() + await expect(info).toContainText('5s 后得自动关闭')Optional follow-up (outside the selected lines):
// Verify it auto-closes within ~7s. await expect(page.getByText('自定义消息的内容可关闭，5s 后得自动关闭', { exact: false })).toHaveCount(0, { timeout: 7000 })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
examples/sites/demos/pc/app/modal/basic-usage.spec.ts(1 hunks)examples/sites/demos/pc/app/modal/message-close.spec.ts(1 hunks)examples/sites/demos/pc/app/modal/message-id.spec.ts(1 hunks)examples/sites/demos/pc/app/modal/modal-fn-slots.spec.ts(1 hunks)examples/sites/demos/pc/app/modal/modal-header.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/sites/demos/pc/app/modal/modal-fn-slots.spec.ts
⏰ 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: PR Unit Test
- GitHub Check: PR E2E Test (pnpm test:e2e3)
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
在vue2版本情况下，函数式插槽写法无响应，兼容vue2写法
What is the current behavior?
Issue Number: #3450
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
Bug Fixes
Tests