test(ui): add tests for mobile menu default closed state#2195
test(ui): add tests for mobile menu default closed state#2195howwohmm wants to merge 5 commits intonpmx-dev:mainfrom
Conversation
…vior Covers issue requirements: - Menu is closed by default (dialog not in DOM) - Opens when prop is set to true - Closes when prop changes back to false - Emits update:open on backdrop click - Emits update:open on close button click Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughA new Vitest test suite for the Nuxt Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 24804db7-68df-4724-9496-6879bbe8aa62
📒 Files selected for processing (1)
test/nuxt/components/Header/MobileMenu.spec.ts
| mockNuxtImport('useConnector', () => () => ({ | ||
| isConnected: computed(() => false), | ||
| npmUser: computed(() => null), | ||
| avatar: computed(() => null), | ||
| })) |
There was a problem hiding this comment.
Align the useConnector mock shape with the real composable contract.
Line 7 mocks isConnected, but the production useConnector contract appears to use connected. This mock can hide a real integration mismatch and give false confidence.
Suggested adjustment
mockNuxtImport('useConnector', () => () => ({
- isConnected: computed(() => false),
+ connected: computed(() => false),
npmUser: computed(() => null),
avatar: computed(() => null),
}))If MobileMenu.client.vue expects isConnected, consider aliasing at destructure time there (e.g. connected: isConnected) so both runtime and tests reflect the same contract.
📝 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.
| mockNuxtImport('useConnector', () => () => ({ | |
| isConnected: computed(() => false), | |
| npmUser: computed(() => null), | |
| avatar: computed(() => null), | |
| })) | |
| mockNuxtImport('useConnector', () => () => ({ | |
| connected: computed(() => false), | |
| npmUser: computed(() => null), | |
| avatar: computed(() => null), | |
| })) |
The Nuxt auto-generated name for Header/MobileMenu.client.vue is HeaderMobileMenu, not MobileMenuClient. Also removes unused `ref` import. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/nuxt/components/Header/MobileMenu.spec.ts (1)
88-89: Consider a more type-safe pattern for array access.Lines 89 and 105 use the non-null assertion operator
!to accesswrapper.emitted('update:open')![0]. Whilst the preceding.toBeTruthy()check ensures the array exists, it doesn't explicitly verify that index[0]exists.♻️ Suggested refactor for stricter type safety
For lines 88-89:
- expect(wrapper.emitted('update:open')).toBeTruthy() - expect(wrapper.emitted('update:open')![0]).toEqual([false]) + const emitted = wrapper.emitted('update:open') + expect(emitted).toBeTruthy() + expect(emitted?.[0]).toEqual([false])For lines 104-105:
- expect(wrapper.emitted('update:open')).toBeTruthy() - expect(wrapper.emitted('update:open')![0]).toEqual([false]) + const emitted = wrapper.emitted('update:open') + expect(emitted).toBeTruthy() + expect(emitted?.[0]).toEqual([false])As per coding guidelines, strictly type-safe code should check when accessing an array value by index.
Also applies to: 104-105
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1fe96943-99c4-4330-9b64-7f0e9f132fb1
📒 Files selected for processing (1)
test/nuxt/components/Header/MobileMenu.spec.ts
Switch from dynamic `await import('#components')` to static top-level
import to match the pattern used by HeaderConnectorModal.spec.ts.
Add required `type: 'link'` to NavigationLink test data.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
1 participant
🔗 Linked issue
Closes #1505
🧭 Context
The mobile menu had no tests verifying its default state or basic open/close behavior.
📚 Description
Adds a test suite for
MobileMenu.client.vuecovering:openis falserole="dialog"andaria-modal="true"appears whenopenis trueopengoes from true to falseupdate:openwithfalseupdate:openwithfalseMocks
useConnector,useAtproto, anduseFocusTrapto isolate the menu logic. Follows existing patterns fromHeaderConnectorModal.spec.tsandTooltip.spec.ts(Teleport + cleanup).What it doesn't change