[BUG-2] Multi-element useEvents leaks listeners and duplicates handlers

[shroy]

Severity: P0 · Batch: B2 (useEvents)

Location

src/hooks/useEvents.js:84 (currentHandlers.set(eventName, handlerWithIndex) inside elements.forEach), removal loops at :51-56 and :97-104

Root cause

currentHandlers is keyed by event name only; with N elements, each iteration overwrites the previous element's wrapper. Cleanup calls removeEventListener(event, lastWrapper) on all elements — a no-op for elements 0..N-2.

Failure scenario

Cleanup leaves N-1 live listeners per event; every reactive re-bind (updateEventListeners) stacks an additional handler on all but the last element.

Proposed fix

Key by element+event, e.g. Map<EventTarget, Map<string, Function>>, or store {element, eventName, wrapper} tuples and remove exactly what was added.

Verification recipe

Spec: useEvents([el1, el2, el3], { click: spy }) in a context, run the returned cleanup, dispatch click on el1, assert spy not called. Expected on unmodified code: spy IS called (listener leaked) → confirmed.

[shroy]

Phase 1 verification: CONFIRMED

Method: runtime repro (vitest + jsdom)

The bug is real and reproduces exactly as described in the finding. On unmodified code, src/hooks/useEvents.js:84 (currentHandlers.set(eventName, handlerWithIndex)) keys the map by event name only. Inside the outer elements.forEach, each iteration overwrites the previous element's wrapper, so after registration currentHandlers holds only the last element's wrapper. Cleanup (lines 97-104) then calls removeEventListener(event, lastWrapper) on every element — a no-op for elements 0..N-2, whose listeners leak.

Repro spec (src/tests/__verify__/issue-13.spec.js)

import { describe, it, expect, vi } from 'vitest'
import { useEvents } from '../../hooks/useEvents.js'
import { withHookContext } from '../../core/hookContext.js'

/**
 * BUG-2 / issue #13 verification.
 *
 * `useEvents([el1, el2, el3], { click: spy })` keys its internal
 * `currentHandlers` map by event name only, so the outer `elements.forEach`
 * loop overwrites each previous element's wrapper. On cleanup, only the LAST
 * element's wrapper is known, so removeEventListener is a no-op for elements
 * 0..N-2 and their listeners leak.
 *
 * These specs assert the CORRECT / documented behavior: after cleanup, NO
 * element should still invoke the handler. They FAIL on unmodified code iff
 * the bug is real.
 *
 * BUG-34 note: withHookContext swallows exceptions, so we only capture the
 * cleanup fn inside the context and run cleanup + all assertions OUTSIDE it.
 */
describe('BUG-2 / issue #13 — multi-element useEvents cleanup', () => {
  it('cleanup removes the click listener from EVERY element (no leak)', () => {
    const el1 = document.createElement('button')
    const el2 = document.createElement('button')
    const el3 = document.createElement('button')
    document.body.append(el1, el2, el3)

    const spy = vi.fn()

    // Faithful to the recipe: register "in a context". Plain-handler useEvents
    // does not touch useEffect, but we wrap anyway. Capture cleanup outward.
    let cleanup
    withHookContext(el1, () => {
      cleanup = useEvents([el1, el2, el3], { click: spy })
    })

    // Sanity: while listeners are live, all three elements fire the handler.
    el1.dispatchEvent(new Event('click'))
    el2.dispatchEvent(new Event('click'))
    el3.dispatchEvent(new Event('click'))
    expect(spy).toHaveBeenCalledTimes(3)

    spy.mockClear()

    // Run cleanup — should remove the listener from ALL elements.
    cleanup()

    // Dispatch again on every element. Correct behavior: zero calls.
    el1.dispatchEvent(new Event('click'))
    el2.dispatchEvent(new Event('click'))
    el3.dispatchEvent(new Event('click'))

    // The decisive assertion from the recipe: el1 (a non-last element) must
    // NOT fire after cleanup. On buggy code, el1 and el2 still fire (2 calls).
    expect(spy).toHaveBeenCalledTimes(0)

    document.body.replaceChildren()
  })

  it('cleanup verified against removeEventListener spies per element', () => {
    const el1 = document.createElement('button')
    const el2 = document.createElement('button')
    const el3 = document.createElement('button')

    const add1 = vi.spyOn(el1, 'addEventListener')
    const add2 = vi.spyOn(el2, 'addEventListener')
    const add3 = vi.spyOn(el3, 'addEventListener')
    const remove1 = vi.spyOn(el1, 'removeEventListener')
    const remove2 = vi.spyOn(el2, 'removeEventListener')
    const remove3 = vi.spyOn(el3, 'removeEventListener')

    const spy = vi.fn()

    const cleanup = useEvents([el1, el2, el3], { click: spy })

    // Each element got its OWN wrapper attached.
    const wrapper1 = add1.mock.calls.find(([name]) => name === 'click')[1]
    const wrapper2 = add2.mock.calls.find(([name]) => name === 'click')[1]
    const wrapper3 = add3.mock.calls.find(([name]) => name === 'click')[1]

    expect(wrapper1).not.toBe(wrapper2)
    expect(wrapper2).not.toBe(wrapper3)

    cleanup()

    // Correct behavior: each element must have its OWN wrapper removed.
    expect(remove1).toHaveBeenCalledWith('click', wrapper1)
    expect(remove2).toHaveBeenCalledWith('click', wrapper2)
    expect(remove3).toHaveBeenCalledWith('click', wrapper3)
  })
})

Test output (decisive failures)

 ❯ src/tests/__verify__/issue-13.spec.js  (2 tests | 2 failed) 12ms

 FAIL  ... > cleanup removes the click listener from EVERY element (no leak)
AssertionError: expected "spy" to be called +0 times, but got 2 times
 ❯ src/tests/__verify__/issue-13.spec.js:55:17
     53|     // The decisive assertion from the recipe: el1 (a non-last element…
     54|     // NOT fire after cleanup. On buggy code, el1 and el2 still fire (…
     55|     expect(spy).toHaveBeenCalledTimes(0)

 FAIL  ... > cleanup verified against removeEventListener spies per element
AssertionError: expected "removeEventListener" to be called with arguments: [ Array(2) ]
Number of calls: 1
 ❯ src/tests/__verify__/issue-13.spec.js:87:21
     87|     expect(remove1).toHaveBeenCalledWith('click', wrapper1)

 Test Files  1 failed (1)
      Tests  2 failed (2)

Interpretation

Both specs assert the documented behavior and fail on unmodified code. Test 1 shows that after cleanup(), the two non-last elements (el1, el2) still fire the handler — 2 leaked listeners where 0 were expected. Test 2 shows each element only ever has removeEventListener called once with the last element's wrapper (wrapper3), never its own wrapper1/wrapper2, so cleanup removes the wrong handler on all but the final element. This exactly matches the P0 finding: N-1 leaked listeners per event, plus handler duplication on every reactive re-bind.