fix: only trigger the step destroy event on real teardown

[chuckcarpenter]

Fixes #3443.

destroy has been firing every time an already-shown step is shown again, which makes it unusable as a teardown hook. @remiHau hit this trying to open a three-dots menu in beforeShowPromise and close it in when: { destroy } — going back and then forward closed the menu right after opening it.

What was happening

_setupElements() rebuilds a step's element on every show, and it tore down the previous element by calling the public destroy():

_setupElements() {
  if (!isUndefined(this.el)) {
    this.destroy();      // emits `destroy`
  }

  this.el = this._createTooltipContent();
  // ...
}

So the second show of any step emitted before-show → destroy → show.

We already have _teardownElements(), which exists to do that teardown without emitting the public event, and updateStepOptions() was already using it. _setupElements() should have been too. destroy now means one thing — the step is gone for good — and fires from step.destroy(), tour.removeStep(id), and once per step from _done() on complete/cancel.

Note for the record: the guard predates 0fff410 (#583) by a couple of months (#430). What #583 changed was making _show() call _setupElements() unconditionally, which is what started exercising it on every show.

Two things that had to come along

advanceOn was unbinding via the destroy event. bindAdvance registered step.on('destroy', () => removeEventListener(...)) as its only cleanup path, so dropping the per-show event would have leaked a DOM listener on every show. It now returns a cleanup function that _teardownElements() calls, which also closes out the old TODO: this should also bind/unbind on show/hide.

_teardownElements() wasn't idempotent. It clears _originalTabIndexes but never resets target, and _restoreOriginalTabIndexes() removes the attribute outright on a map miss — so running teardown twice permanently dropped a target's original tabindex. This was already reachable through updateStepOptions(), independent of this issue. Guarding on isHTMLElement(this.el) covers all three el states (undefined / null / element) in one predicate and fixes it.

Behavior change

Anyone counting destroy invocations will now see fewer of them. I'm treating this as a bug rather than a break: the per-show emission was documented nowhere, and it contradicts what destroy() is documented to mean. Labeling bug.

Supersedes #3455

Thanks to @remiHau for the diagnosis and for taking a run at it. #3455 adds an isNull check to the same guard, but it doesn't fix the reported symptom — hide() never nulls el, it only sets hidden = true, so on a plain back → next el is still an HTMLElement and destroy() fires exactly as before. It only suppresses the second destroy once you've already called destroy() yourself from a hide handler, which makes the workaround viable rather than removing the need for it. Going with _teardownElements() instead of loosening the guard, since the real problem is a public event firing from a private code path.

Tests

• Re-showing a step doesn't trigger destroy; destroy fires exactly once per step on complete
• A reproduction of the reported scenario: target created in beforeShowPromise, cleanup in when: { destroy }, asserted across back → next and on both complete and cancel
• advanceOn listeners don't accumulate across shows (adds == removes)
• Original tabindex survives updateStepOptions()
• Reworked the two tests that asserted on the old behavior — the _setupElements one and bind.spec.js's "calls removeEventListener when destroyed", which now tests the returned cleanup

All five new tests fail against the old code, so they're actually guarding something. Lint, types:check, 190 unit tests and 44 Cypress specs pass.

Also documented the step lifecycle in the usage guide, with a table of which events fire when — that clearly wasn't discoverable.

Summary by CodeRabbit

• Documentation

  • Clarified the differences between hiding and permanently destroying steps.
  • Documented methods for updating options and accessing elements and targets.
  • Added detailed lifecycle event timing and ordering guidance.

• Bug Fixes

  • Prevented temporary step rebuilding from triggering permanent-destruction events.
  • Improved cleanup of advance listeners and preserved element accessibility settings during updates.
  • Added more reliable handling when step targets are recreated asynchronously.

[vercel]

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

Project	Deployment	Actions	Updated (UTC)
shepherd-docs	Ready	Preview	Aug 12, 2026 1:29pm

1 Skipped Deployment

Project	Deployment	Actions	Updated (UTC)
shepherd-landing	Skipped		Aug 12, 2026 1:29pm

[coderabbitai]

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: f7df1cf3-9fec-42bb-ba42-aeaa17c853b9

📥 Commits

Reviewing files that changed from the base of the PR and between ab367b9 and c46adfd.

📒 Files selected for processing (1)

• docs-src/src/content/docs/guides/usage.md

🚧 Files skipped from review as they are similar to previous changes (1)

• docs-src/src/content/docs/guides/usage.md

---

📝 Walkthrough

Walkthrough

Step element rebuilding now uses internal teardown instead of public destruction. Advance listeners have explicit cleanup. Documentation and tests define and verify the updated lifecycle event timing.

Changes

Step lifecycle

Layer / File(s)	Summary
Internal teardown and listener cleanup shepherd.js/src/step.ts, shepherd.js/src/utils/bind.ts	Step element replacement tears down existing elements without emitting destroy. bindAdvance returns cleanup functions, which Step invokes during teardown.
Lifecycle contract and validation docs-src/src/content/docs/guides/usage.md, shepherd.js/test/unit/step.spec.js, shepherd.js/test/unit/utils/bind.spec.js	The documentation defines method and event timing. Tests cover final destruction, listener cleanup, element recreation, missing bindings, and option updates.

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

Sequence Diagram(s)

sequenceDiagram
  participant Step
  participant bindAdvance
  participant DOM
  Step->>Step: _setupElements()
  Step->>DOM: Tear down existing elements
  Step->>bindAdvance: bindAdvance(step)
  bindAdvance->>DOM: Install advance listener
  Step->>DOM: Invoke stored cleanup during teardown

Loading

Possibly related PRs

• shipshapecode/shepherd#3471: Related Step element lifecycle and asynchronous target recreation changes.

Suggested labels: bug

🚥 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 summarizes the primary fix: emit the step destroy event only during actual teardown.
Linked Issues check	✅ Passed	The changes address issue #3443 by preventing destroy events during step re-show and preserving cleanup for actual destruction.
Out of Scope Changes check	✅ Passed	The implementation, tests, and documentation changes directly support lifecycle teardown, listener cleanup, and destroy-event behavior.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch claude/shepherd-issue-3443-b19598

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with 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.

Inline comments:
In `@docs-src/src/content/docs/guides/usage.md`:
- Around line 368-372: Update the documentation around Step.destroy to scope the
once-only claim to element recreation: state that re-showing does not emit
destroy, while repeated explicit destroy() calls may emit it each time. Do not
claim that destroy fires only once unless the implementation is also changed to
make destroy() idempotent.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

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

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 60e00895-c4b8-4bdc-8eec-90809d87be1a

📥 Commits

Reviewing files that changed from the base of the PR and between 4b101e1 and bb50a42.

📒 Files selected for processing (5)

• docs-src/src/content/docs/guides/usage.md
• shepherd.js/src/step.ts
• shepherd.js/src/utils/bind.ts
• shepherd.js/test/unit/step.spec.js
• shepherd.js/test/unit/utils/bind.spec.js

[qltysh]

Coverage Impact

⬆️ Merging this pull request will increase total coverage on main by 0.6%.

Modified Files with Diff Coverage (2)

Rating	File	% Diff	Uncovered Line #s
	shepherd.js/src/step.ts	100.0%
	shepherd.js/src/utils/bind.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.