fix: not call destroy on destroyed step

[remiHau]

Currently on function _setupElements of step, the destroy function is called if this.el is undefined but the destroy method set this.el to null. So function destroy is called also on destroyed element which trigger unnecessary 'destroy' event.

Another possible change would be to change the condition in _setupElements to check if this.el is either undefined or null.

This is linked with reported issue #3443

Summary by CodeRabbit

• Bug Fixes

  • Enhanced step teardown to properly track the destroyed state and avoid redundant destroy operations on already-destroyed steps.

• Tests

  • Added test coverage verifying that destroy operations are not called redundantly on steps that have already been destroyed, ensuring more reliable cleanup behavior.

[vercel]

Someone is attempting to deploy a commit to the shipshapecode Team on Vercel.

A member of the Team first needs to authorize it.

[coderabbitai]

📝 Walkthrough

Walkthrough

In step.ts, the _teardownElements() method now sets this.el to undefined instead of null after removing the tooltip element. A companion test is added to step.spec.js verifying that _setupElements() does not invoke destroy on an already-destroyed step.

Changes

Step teardown sentinel fix

Layer / File(s)	Summary
el teardown sentinel and guard test shepherd.js/src/step.ts, shepherd.js/test/unit/step.spec.js	_teardownElements() assigns undefined (instead of null) to this.el after removing the tooltip DOM element; a new unit test constructs a step, destroys it, then calls _setupElements() and asserts the replaced destroy spy was never invoked.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Poem

A bunny once hopped through the code one night,
Found null where undefined ought to sit right.
One character swapped, the sentinel aligned,
A test now confirms no ghost-destroy designed.
🐇✨ The warren is tidy, all is defined!

🚥 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 accurately describes the main change: preventing the destroy method from being called on already-destroyed steps, which aligns with both the code changes (modifying teardown logic) and the PR objectives.
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.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

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/step.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.4.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

• X
• Mastodon
• Reddit
• LinkedIn

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

[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 `@shepherd.js/src/step.ts`:
- 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.

🪄 Autofix (Beta)

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

Run ID: 82196bde-61fb-41d7-8d5f-b3b9cfd8b837

📥 Commits

Reviewing files that changed from the base of the PR and between e256dcb and 98217ff.

📒 Files selected for processing (2)

• shepherd.js/src/step.ts
• shepherd.js/test/unit/step.spec.js

[coderabbitai]

⚠️ 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.