packages/vscode-preview/src/interceptExternalLinks.spec.ts (1)

91-105: Consider table-driven coverage for all interactive selector cases.

This test currently validates only button. A small parameterized test over [role="button"], input, select, and textarea would better protect the selector contract.

As per coding guidelines, "Aim to cover new features with relevant tests and keep test names descriptive".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/vscode-preview/src/interceptExternalLinks.spec.ts` around lines 91 -
105, The test only checks the 'button' case of the interactive selector and
should be converted into a table-driven/parameterized test to cover
'[role="button"]', 'input', 'select', and 'textarea' as well; update the spec
that calls handleExternalLinkClick(mockEvent(target), openUrl) to iterate over
an array of selector-tag pairs (or selector values) and for each stub
mockEvent().closest to return the appropriate element for the interactive
selector and an anchor for 'a[href]', then assert openUrl was not called for
every case using the same mock openUrl; reference the existing test helpers
mockEvent, openUrl and the function under test handleExternalLinkClick and reuse
the selector string 'button, [role="button"], input, select, textarea' to drive
the cases.

packages/vscode-preview/src/interceptExternalLinks.spec.ts (2)

91-111: Make the interactive-element parameterized test validate each selector independently.

At Line 100, the mock currently keys off selector.includes('button') && selector.includes('input'), so each it.each case effectively exercises the same branch. This can miss regressions where one selector is dropped but others remain.

Suggested test-mock refinement

   it.each([
     'button',
     '[role="button"]',
     'input',
     'select',
     'textarea',
   ])('skips interactive element inside link: %s', (tag) => {
     const target = {
       closest: vi.fn((selector: string) => {
-        if (selector.includes('button') && selector.includes('input')) {
+        const selectors = selector.split(',').map(s => s.trim())
+        if (selectors.includes(tag)) {
           return { tagName: tag.toUpperCase() }
         }
         if (selector === 'a[href]') {
           return { getAttribute: () => 'https://example.com' }
         }
         return null
       }),
     }

As per coding guidelines: "Aim to cover new features with relevant tests and keep test names descriptive."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/vscode-preview/src/interceptExternalLinks.spec.ts` around lines 91 -
111, The test's mock for Element.closest always triggers the same branch because
it checks for selector.includes('button') && selector.includes('input'), so each
it.each case isn't validated independently; update the mock in the 'skips
interactive element inside link' test so closest examines the exact selector
argument (e.g., compare selector === expectedSelector or check
selector.includes(tag) mapped to tag names) and returns an element with tagName:
tag.toUpperCase() only when the selector matches that specific interactive
selector, otherwise return the anchor for 'a[href]' or null; this ensures
handleExternalLinkClick (and the mockEvent/openUrl expectations) are exercised
per selector case rather than all hitting one branch.

---

151-153: Tighten listener wiring assertions to catch extra registrations/removals.

Using toContainEqual verifies presence but still passes if unintended extra add/remove calls are introduced. Prefer exact assertions for add/remove subsets.

Suggested assertion tightening

-    expect(calls).toContainEqual({ type: 'click', capture: true, action: 'add' })
-    expect(calls).toContainEqual({ type: 'auxclick', capture: false, action: 'add' })
+    expect(calls.filter(c => c.action === 'add')).toEqual([
+      { type: 'click', capture: true, action: 'add' },
+      { type: 'auxclick', capture: false, action: 'add' },
+    ])

@@
-    expect(calls).toContainEqual({ type: 'click', capture: true, action: 'remove' })
-    expect(calls).toContainEqual({ type: 'auxclick', capture: false, action: 'remove' })
+    expect(calls.filter(c => c.action === 'remove')).toEqual([
+      { type: 'click', capture: true, action: 'remove' },
+      { type: 'auxclick', capture: false, action: 'remove' },
+    ])

Also applies to: 166-167

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/vscode-preview/src/interceptExternalLinks.spec.ts` around lines 151
- 153, The current tests use toContainEqual on the `calls` array (e.g., the
expectations checking { type: 'click', capture: true, action: 'add' } and {
type: 'auxclick', capture: false, action: 'add' }) which only asserts presence
and misses extra unwanted add/remove registrations; change these to assert exact
subsets by filtering `calls` by `action` (e.g., const adds = calls.filter(c =>
c.action === 'add')) and then assert the exact array equality with toEqual on
the expected add objects, and do the same for removes (and update the other
occurrences around the second pair of assertions at the similar spot referenced
in the comment), ensuring no extra registrations or removals are present.

packages/vscode-preview/src/interceptExternalLinks.spec.ts (2)

135-143: Redundant stopPropagation override.

Since mockEvent now already provides stopPropagation: vi.fn() (line 11), the spread-and-override here is unnecessary — you can simplify to just const e = mockEvent(mockAnchor('https://example.com')) and assert on e.stopPropagation directly.

♻️ Proposed simplification

   it('also calls stopPropagation on intercepted links', () => {
-    const e = {
-      ...mockEvent(mockAnchor('https://example.com')),
-      stopPropagation: vi.fn(),
-    } as unknown as MouseEvent
+    const e = mockEvent(mockAnchor('https://example.com'))
     handleExternalLinkClick(e, openUrl)
     expect(e.stopPropagation).toHaveBeenCalled()
     expect(openUrl).toHaveBeenCalledWith('https://example.com')
   })

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/vscode-preview/src/interceptExternalLinks.spec.ts` around lines 135
- 143, The test redundantly overrides stopPropagation when mockEvent already
supplies it; update the test in interceptExternalLinks.spec.ts by replacing the
spread override with a direct call to mockEvent: set const e =
mockEvent(mockAnchor('https://example.com')) (cast to MouseEvent if necessary),
then call handleExternalLinkClick(e, openUrl) and keep the assertions
expect(e.stopPropagation).toHaveBeenCalled() and
expect(openUrl).toHaveBeenCalledWith('https://example.com'); remove the {...,
stopPropagation: vi.fn()} spread override.

---

97-116: Interactive-selector matching is overly loose for button.

The predicate selectors.some(s => s === tag || s.includes(tag)) causes the button case to also match when iterating over a selector list containing [role="button"] (since the substring "button" is present). The test still passes because both branches lead to return { tagName: ... } and the assertions only check that openUrl wasn't called, but the mock's behavior no longer faithfully represents what closest(interactiveSelector) would return for a plain <button> vs. a [role="button"]. Consider matching only when s === tag to keep the mock precise:

♻️ Proposed tightening

-        const selectors = selector.split(',').map(s => s.trim())
-        if (selectors.some(s => s === tag || s.includes(tag))) {
-          return { tagName: tag.toUpperCase() }
-        }
+        const selectors = selector.split(',').map(s => s.trim())
+        if (selectors.includes(tag)) {
+          return { tagName: tag.replace(/\W/g, '').toUpperCase() }
+        }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/vscode-preview/src/interceptExternalLinks.spec.ts` around lines 97 -
116, The test's mock for Element.closest is too permissive for the 'button'
case: inside the closest callback in interceptExternalLinks.spec.ts (the mock
tied to mockEvent/target used by handleExternalLinkClick and openUrl) change the
selector-match predicate so it only matches when a selector exactly equals the
tag (i.e., use strict equality rather than substring includes) so that
`[role="button"]` does not incorrectly match 'button'; keep the rest of the mock
(returning tagName or the 'a[href]' branch) and leave assertions unchanged.