test/components/CommandPalette.spec.ts (1)

171-205: ⚡ Quick win

Wrap mock restoration in try-finally to prevent test pollution.

If an assertion fails before line 204, the original scrollIntoView implementation won't be restored, potentially affecting subsequent tests.

🛡️ Recommended fix to ensure cleanup

  it('re-highlights without scrolling the page when results arrive without focus', async () => {
    const scrollSpy = vi.fn()
    const original = window.HTMLElement.prototype.scrollIntoView
    window.HTMLElement.prototype.scrollIntoView = scrollSpy
+   try {
+     const wrapper = await mountSuspended(CommandPalette, {
+       props: { groups: [{ id: 'users', items: [] }], autofocus: false } as any,
+       attachTo: document.body
+     })
+     await new Promise(resolve => setTimeout(resolve, 60))

-   const wrapper = await mountSuspended(CommandPalette, {
-     props: { groups: [{ id: 'users', items: [] }], autofocus: false } as any,
-     attachTo: document.body
-   })
-   await new Promise(resolve => setTimeout(resolve, 60))
-
-   // Items arrive asynchronously while the palette is not focused
-   await wrapper.setProps({
-     groups: [{ id: 'users', items: Array.from({ length: 10 }, (_, i) => ({ label: `User ${i}` })) }]
-   } as any)
-   await new Promise(resolve => setTimeout(resolve, 60))
-
-   // First item is highlighted for keyboard entry, but the page was not scrolled to it.
-   expect(wrapper.find('[data-highlighted]').exists()).toBe(true)
-   expect(scrollSpy).not.toHaveBeenCalled()
-
-   // Once the palette has focus, a results change scrolls the highlight into view.
-   ;(wrapper.find('input').element as HTMLInputElement).focus()
-   await wrapper.setProps({
-     groups: [{ id: 'users', items: Array.from({ length: 8 }, (_, i) => ({ label: `Person ${i}` })) }]
-   } as any)
-   await new Promise(resolve => setTimeout(resolve, 60))
-
-   expect(scrollSpy).toHaveBeenCalled()
-
-   window.HTMLElement.prototype.scrollIntoView = original
+     // Items arrive asynchronously while the palette is not focused
+     await wrapper.setProps({
+       groups: [{ id: 'users', items: Array.from({ length: 10 }, (_, i) => ({ label: `User ${i}` })) }]
+     } as any)
+     await new Promise(resolve => setTimeout(resolve, 60))
+
+     // First item is highlighted for keyboard entry, but the page was not scrolled to it.
+     expect(wrapper.find('[data-highlighted]').exists()).toBe(true)
+     expect(scrollSpy).not.toHaveBeenCalled()
+
+     // Once the palette has focus, a results change scrolls the highlight into view.
+     ;(wrapper.find('input').element as HTMLInputElement).focus()
+     await wrapper.setProps({
+       groups: [{ id: 'users', items: Array.from({ length: 8 }, (_, i) => ({ label: `Person ${i}` })) }]
+     } as any)
+     await new Promise(resolve => setTimeout(resolve, 60))
+
+     expect(scrollSpy).toHaveBeenCalled()
+   } finally {
+     window.HTMLElement.prototype.scrollIntoView = original
+   }
  })

🤖 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 `@test/components/CommandPalette.spec.ts` around lines 171 - 205, The test
replaces window.HTMLElement.prototype.scrollIntoView with scrollSpy but only
restores it at the end, so a failing assertion can leak the mock; wrap the test
body that manipulates scrollIntoView (the original/scrollSpy assignment, the
mountSuspended call, wrapper.setProps and the focus/assertions) in a try-finally
and move window.HTMLElement.prototype.scrollIntoView = original into the finally
block so the original implementation is always restored even if assertions fail;
reference the existing scrollSpy variable, the original variable, and the
wrapper/find('input') focus sequence when making the change.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes