From ddf93f35894ef9905655dd2200b6ef01a0df39f3 Mon Sep 17 00:00:00 2001
From: Chuck Carpenter <charleswcarpenter3@gmail.com>
Date: Thu, 13 Aug 2026 09:53:08 +0200
Subject: [PATCH] fix: do not skip event handlers registered after a `once`
 handler

`trigger()` spliced `this.bindings[event]` while iterating it with
`forEach`. Removing a spent `once` binding shifted every later binding
down one, but the loop still advanced, so the handler that moved into
the vacated slot was never called.

Iterate over a copy and remove `once` bindings by identity rather than
by loop index. Removing by index into a snapshot is not enough on its
own: the indexes drift once more than one `once` handler is registered
for the same event, leaving a spent binding behind to fire again on the
next trigger.

`off()` had the same splice-during-iteration bug, leaving one binding
behind when the same handler was registered more than once. Rewrite it
as a filter.

Co-authored-by: Brett Ausmeier <brett@decodedev.co.za>
---
 shepherd.js/src/evented.ts            | 38 +++++++++++------
 shepherd.js/test/unit/evented.spec.js | 61 ++++++++++++++++++++++++++-
 2 files changed, 85 insertions(+), 14 deletions(-)

diff --git a/shepherd.js/src/evented.ts b/shepherd.js/src/evented.ts
index 95f55500f..ae30d73be 100644
--- a/shepherd.js/src/evented.ts
+++ b/shepherd.js/src/evented.ts
@@ -51,18 +51,18 @@ export class Evented {
    * @returns
    */
   off(event: string, handler?: AnyHandler) {
-    if (isUndefined(this.bindings) || isUndefined(this.bindings[event])) {
+    const bindings = this.bindings?.[event];
+
+    if (isUndefined(bindings)) {
       return this;
     }
 
     if (isUndefined(handler)) {
       delete this.bindings[event];
     } else {
-      this.bindings[event]?.forEach((binding, index) => {
-        if (binding.handler === handler) {
-          this.bindings[event]?.splice(index, 1);
-        }
-      });
+      this.bindings[event] = bindings.filter(
+        (binding) => binding.handler !== handler
+      );
     }
 
     return this;
@@ -76,18 +76,30 @@ export class Evented {
    */
   // eslint-disable-next-line @typescript-eslint/no-explicit-any
   trigger(event: string, ...args: any[]) {
-    if (!isUndefined(this.bindings) && this.bindings[event]) {
-      this.bindings[event]?.forEach((binding, index) => {
-        const { ctx, handler, once } = binding;
+    const bindings = this.bindings?.[event];
+
+    if (isUndefined(bindings)) {
+      return this;
+    }
+
+    // Iterate over a copy, since handlers may add or remove bindings while we
+    // are dispatching.
+    for (const binding of bindings.slice()) {
+      const { ctx, handler, once } = binding;
+
+      const context = ctx || this;
 
-        const context = ctx || this;
+      handler.apply(context, args as []);
 
-        handler.apply(context, args as []);
+      if (once) {
+        // Look the binding up by identity rather than by loop index, since
+        // indexes shift as bindings are removed.
+        const index = this.bindings[event]?.indexOf(binding) ?? -1;
 
-        if (once) {
+        if (index !== -1) {
           this.bindings[event]?.splice(index, 1);
         }
-      });
+      }
     }
 
     return this;
diff --git a/shepherd.js/test/unit/evented.spec.js b/shepherd.js/test/unit/evented.spec.js
index 41b4c0935..9193367f2 100644
--- a/shepherd.js/test/unit/evented.spec.js
+++ b/shepherd.js/test/unit/evented.spec.js
@@ -38,6 +38,48 @@ describe('Evented', () => {
         step: { id: 'test', text: 'A step' }
       });
     });
+
+    it('does not skip event bindings after removing an event binding', () => {
+      testEvent.once('testOn', () => true);
+      const handlerSpy = vi.fn();
+      testEvent.on('testOn', handlerSpy);
+
+      testEvent.trigger('testOn');
+
+      expect(handlerSpy).toHaveBeenCalled();
+    });
+
+    it('calls every once handler and removes all of them', () => {
+      const firstSpy = vi.fn();
+      const secondSpy = vi.fn();
+      const thirdSpy = vi.fn();
+      testEvent.once('multipleOnce', firstSpy);
+      testEvent.once('multipleOnce', secondSpy);
+      testEvent.once('multipleOnce', thirdSpy);
+
+      testEvent.trigger('multipleOnce');
+
+      expect(firstSpy).toHaveBeenCalledTimes(1);
+      expect(secondSpy).toHaveBeenCalledTimes(1);
+      expect(thirdSpy).toHaveBeenCalledTimes(1);
+      expect(
+        testEvent.bindings.multipleOnce,
+        'no spent once bindings left behind'
+      ).toHaveLength(0);
+    });
+
+    it('only calls a once handler for the first trigger', () => {
+      const onceSpy = vi.fn();
+      const onSpy = vi.fn();
+      testEvent.once('mixed', onceSpy);
+      testEvent.on('mixed', onSpy);
+
+      testEvent.trigger('mixed');
+      testEvent.trigger('mixed');
+
+      expect(onceSpy).toHaveBeenCalledTimes(1);
+      expect(onSpy).toHaveBeenCalledTimes(2);
+    });
   });
 
   describe('off()', () => {
@@ -60,6 +102,23 @@ describe('Evented', () => {
       ).toBe(1);
     });
 
+    it('removes every binding for a handler registered more than once', () => {
+      const handler = () => {};
+      testEvent.on('testOn', handler);
+      testEvent.on('testOn', handler);
+      expect(
+        testEvent.bindings.testOn.length,
+        '3 event listeners for testOn'
+      ).toBe(3);
+
+      testEvent.off('testOn', handler);
+
+      expect(
+        testEvent.bindings.testOn.length,
+        '1 event listener for testOn'
+      ).toBe(1);
+    });
+
     it('does not remove uncreated events', () => {
       testEvent.off('testBlank');
       expect(
@@ -76,7 +135,7 @@ describe('Evented', () => {
       expect(
         testEvent.bindings.testOnce,
         'custom event removed after one trigger'
-      ).toBeTruthy();
+      ).toHaveLength(0);
     });
   });
 });