Skip to content

fix: not call destroy on destroyed step #3455

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
remiHau wants to merge 1 commit into
base: main
Choose a base branch
from
+14 −1
Open

fix: not call destroy on destroyed step #3455

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion shepherd.js/src/step.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -447,7 +447,7 @@ export class Step extends Evented {

if (isHTMLElement(this.el)) {
this.el.remove();
this.el = null;
this.el = undefined;

@coderabbitai coderabbitai Bot Jun 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Preserve getElement() contract while fixing the guard logic

Line 450 changes the destroyed sentinel to undefined, but getElement() docs/type still indicate destroyed steps return null. That silently changes external behavior. Prefer fixing the guard in _setupElements() to treat both null and undefined as “not mounted,” and keep teardown assigning null.

Suggested patch 
diff --git a/shepherd.js/src/step.ts b/shepherd.js/src/step.ts
@@
-    if (isHTMLElement(this.el)) {
-      this.el.remove();
-      this.el = undefined;
-    }
+    if (isHTMLElement(this.el)) {
+      this.el.remove();
+      this.el = null;
+    }
@@
-    if (!isUndefined(this.el)) {
+    if (this.el != null) {
       this.destroy();
     }
🤖 Prompt for 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.

In `@shepherd.js/src/step.ts` at line 450, The teardown at line 450 assigns
`undefined` to this.el, but the getElement() method's contract and type
signature indicate it returns null for destroyed steps. Revert line 450 to
assign null instead of undefined to maintain the documented contract. Then
update the guard logic in _setupElements() to check if this.el is either null or
undefined (treating both as "not mounted") to handle any edge cases where
undefined might be present from other code paths.

}

this._updateStepTargetOnHide();
Expand Down
13 changes: 13 additions & 0 deletions shepherd.js/test/unit/step.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -441,6 +441,19 @@ describe('Tour | Step', () => {
).toBeTruthy();
});

it('not calls destroy on the step if the step was destroyed', () => {
const step = new Step(tour, {});
let destroyCalled = false;
step.el = document.createElement('a');
step.destroy()
step.destroy = () => (destroyCalled = true);
step._setupElements();
expect(
destroyCalled,
'_setupElements method not called destroy if the step was destroyed'
).toBeFalsy();
});

it('calls destroy on the tooltip if it already exists', () => {
const step = new Step(tour, {});
let destroyCalled = false;
Expand Down