fix: do not skip event handlers registered after a `once` handler by chuckcarpenter · Pull Request #3476 · shipshapecode/shepherd

chuckcarpenter · 2026-08-13T09:23:25Z

Supersedes #3201.

Event handlers registered after a once handler were silently never called. trigger() spliced this.bindings[event] while iterating it with forEach, so removing a spent once binding shifted every later binding down one while the loop still advanced — the handler that moved into the vacated slot got skipped.

@bausmeier reported this in #3201 and proposed iterating over a copy. That fixes the reported case, but not all of them: index becomes an index into the copy while splice still targets the live array, so the indexes drift as soon as more than one once handler is registered for the same event. Three once handlers all fire, but one spent binding survives and fires again on the next trigger.

So this removes once bindings by identity (indexOf) rather than by loop index.

off() had the same splice-during-iteration bug — registering the same handler twice and calling off once left one binding behind. Rewritten as a filter.

Iterating a snapshot also means a handler removed mid-dispatch still runs if it was registered when the event fired, matching Node's EventEmitter.

Tests

Four new cases in evented.spec.js, each confirmed to fail on main and pass here:

a handler registered after a once handler still fires (the originally reported bug)
three once handlers all fire and none are left behind
a once handler fires only on the first of two triggers, while an on handler fires on both
off() removes every binding for a handler registered more than once

I also fixed an existing assertion in the once() block that checked toBeTruthy() under the message "custom event removed after one trigger" — it passed only because an empty array is truthy, so it was never testing what it claimed.

Full unit suite passes (228 tests), types and lint clean.

Summary by CodeRabbit

Bug Fixes
- Improved event handling when listeners are added or removed during event dispatch.
- Ensured one-time listeners are reliably invoked once and cleaned up afterward.
- Corrected removal of duplicate event handlers.
- Improved behavior when triggering events without registered listeners.
Tests
- Added coverage for one-time listeners, duplicate handlers, mixed listener types, and listener cleanup.

`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>

vercel · 2026-08-13T09:23:32Z

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
shepherd-docs	Ready	Preview	Aug 13, 2026 9:24am
shepherd-landing	Ready	Preview	Aug 13, 2026 9:24am

coderabbitai · 2026-08-13T09:23:49Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 336e3a6d-b0df-44b7-bb4a-bf9ce87e4757

📥 Commits

Reviewing files that changed from the base of the PR and between 1d9c664 and ddf93f3.

📒 Files selected for processing (2)

shepherd.js/src/evented.ts
shepherd.js/test/unit/evented.spec.js

📝 Walkthrough

Walkthrough

Evented.off and Evented.trigger now handle listener mutation, duplicate registrations, and one-time handlers without disrupting dispatch. Unit tests cover these behaviors and verify complete cleanup.

Changes

Event binding safety

Layer / File(s)	Summary
Safe binding removal and dispatch `shepherd.js/src/evented.ts`	`off` filters matching handlers. `trigger` uses a binding snapshot, returns when no bindings exist, and removes one-time bindings by object identity.
Binding lifecycle tests `shepherd.js/test/unit/evented.spec.js`	Tests cover one-time handler cleanup, mixed persistent and one-time handlers, duplicate handler removal, and empty binding lists.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Mergeability Score: ⚪ Minimal · up to ddf93

This localized change fixes skipped event handlers and duplicate-handler removal, with unit tests covering the affected cases; no actionable merge-blocking risk remains after normal checks and review.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the primary fix: preventing handlers registered after a once handler from being skipped.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix/evented-handler-skipping

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

shepherd.js/test/unit/evented.spec.js

(node:2) ESLintIgnoreWarning: The ".eslintignore" file is no longer supported. Switch to using the "ignores" property in "eslint.config.js": https://eslint.org/docs/latest/use/configure/migration-guide#ignore-files
(Use node --trace-warnings ... to show where the warning was created)

Oops! Something went wrong! :(

ESLint: 10.8.1

A config object is using the "root" key, which is not supported in flat config system.

Flat configs always act as if they are the root config file, so this key can be safely removed.

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

qltysh · 2026-08-13T09:25:35Z

Coverage Impact

This PR will not change total coverage.

Modified Files with Diff Coverage (1)

Rating	File	% Diff	Uncovered Line #s
	shepherd.js/src/evented.ts	100.0%
	Total	100.0%

🚦 See full report on Qlty Cloud »

🛟 Help

Diff Coverage: Coverage for added or modified lines of code (excludes deleted files). Learn more.
Total Coverage: Coverage for the whole repository, calculated as the sum of all File Coverage. Learn more.
File Coverage: Covered Lines divided by Covered Lines plus Missed Lines. (Excludes non-executable lines including blank lines and comments.)
- Indirect Changes: Changes to File Coverage for files that were not modified in this PR. Learn more.

chuckcarpenter added the bug label Aug 13, 2026

vercel Bot deployed to Preview – shepherd-docs August 13, 2026 09:24 View deployment

vercel Bot deployed to Preview – shepherd-landing August 13, 2026 09:24 View deployment

chuckcarpenter mentioned this pull request Aug 13, 2026

Fix skipping of event handlers #3201

Closed

Navigation Menu

Uh oh!

Uh oh!

fix: do not skip event handlers registered after a `once` handler - #3476

fix: do not skip event handlers registered after a `once` handler#3476
chuckcarpenter wants to merge 1 commit into
mainfrom
fix/evented-handler-skipping

chuckcarpenter commented Aug 13, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

vercel Bot commented Aug 13, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Aug 13, 2026 •

edited

Loading

Walkthrough

Changes

Uh oh!

qltysh Bot commented Aug 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Uh oh!

Conversation

chuckcarpenter commented Aug 13, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Tests

Summary by CodeRabbit

Uh oh!

vercel Bot commented Aug 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented Aug 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

qltysh Bot commented Aug 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

chuckcarpenter commented Aug 13, 2026 •

edited by coderabbitai Bot

Loading

vercel Bot commented Aug 13, 2026 •

edited

Loading

coderabbitai Bot commented Aug 13, 2026 •

edited

Loading