feat: add waitForElement and skipMissingElement step options - #3471

shepherd.js/src/utils/general.ts (2)

73-88: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider reusing one resolution path.

resolveAttachToElement repeats the locator logic in parseAttachTo (lines 41-61). The two implementations can drift. parseAttachTo can call resolveAttachToElement and keep only the logging and option copying.

♻️ Proposed refactor

 export function parseAttachTo(step: Step) {
   const options = step.options.attachTo || {};
   const returnOpts = Object.assign({}, options);
 
-  if (isFunction(returnOpts.element)) {
-    // Bind the callback to step so that it has access to the object, to enable running additional logic
-    returnOpts.element = returnOpts.element.call(step);
-  }
-
-  if (isString(returnOpts.element)) {
-    // Can't override the element in user opts reference because we can't
-    // guarantee that the element will exist in the future.
-    try {
-      returnOpts.element = document.querySelector(
-        returnOpts.element
-      ) as HTMLElement;
-    } catch (_e) {
-      // TODO
-    }
-    if (!returnOpts.element && !step.options.skipMissingElement) {
-      console.error(
-        `The element for this Shepherd step was not found ${options.element}`
-      );
-    }
-  }
+  const needsLookup =
+    isFunction(returnOpts.element) || isString(returnOpts.element);
+
+  if (needsLookup) {
+    // Can't override the element in user opts reference because we can't
+    // guarantee that the element will exist in the future.
+    returnOpts.element = resolveAttachToElement(step) ?? undefined;
+
+    if (!returnOpts.element && !step.options.skipMissingElement) {
+      console.error(
+        `The element for this Shepherd step was not found ${options.element}`
+      );
+    }
+  }
 
   return returnOpts;
 }

Note one behavior difference: the current parseAttachTo only logs when the locator is a string. The refactor above also logs when a function locator returns nothing. Confirm the wanted behavior before you apply it.

🤖 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/utils/general.ts` around lines 73 - 88, Refactor
parseAttachTo to reuse resolveAttachToElement for locator resolution instead of
duplicating its element/function/string handling. Preserve parseAttachTo’s
existing logging and option-copying behavior, and retain the current restriction
that missing-element logging occurs only when the original locator is a string;
do not introduce logging for function locators that return nothing.

---

99-150: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Add a cancellation path for superseded waits.

waitForAttachToElement runs until the element appears or the timeout expires. Tour.show() invalidates a superseded wait only after it settles (shepherd.js/src/tour.ts lines 374-379). Until then, a subtree MutationObserver on document.documentElement, or a 50 ms poll timer, stays active. With a large waitForElement value, several superseded waits can observe every DOM mutation at the same time.

An optional AbortSignal parameter lets the tour release the observer as soon as a newer show() starts.

♻️ Proposed change

 export function waitForAttachToElement(
   step: Step,
-  timeout: number
+  timeout: number,
+  signal?: AbortSignal
 ): Promise<HTMLElement | null> {
   return new Promise((resolve) => {
     const element = resolveAttachToElement(step);
 
-    if (element || !(timeout > 0)) {
+    if (element || !(timeout > 0) || signal?.aborted) {
       resolve(element);
       return;
     }
@@
     const finish = (result: HTMLElement | null) => {
       observer?.disconnect();
+      signal?.removeEventListener('abort', onAbort);
 
       if (pollTimer !== null) {
         clearInterval(pollTimer);
       }
@@
       resolve(result);
     };
+
+    const onAbort = () => finish(null);
+    signal?.addEventListener('abort', onAbort, { once: true });

🤖 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/utils/general.ts` around lines 99 - 150, Update
waitForAttachToElement to accept an optional AbortSignal and add an abort
cancellation path that calls finish without resolving a found element, ensuring
the MutationObserver, polling interval, and timeout are all released immediately
when the signal is already aborted or becomes aborted. Update the Tour.show()
call site to pass the signal for the active wait so superseded waits are
cancelled as soon as a newer show starts.

shepherd.js/test/unit/utils/general.spec.js (1)

293-338: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Restore stubbed globals and mocks in hooks, not in test bodies. Both new test blocks undo global state as the last statement of the test body. If an assertion fails first, that statement never runs, the state leaks into later tests, and the resulting cascading failures hide the original one.

• shepherd.js/test/unit/utils/general.spec.js#L293-L338: move vi.unstubAllGlobals() into an afterEach hook for the waitForAttachToElement() describe block, and remove the two trailing calls.
• shepherd.js/test/unit/tour.spec.js#L1030-L1064: move the console.error spy restore into an afterEach hook, or call vi.restoreAllMocks() there.

🤖 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/test/unit/utils/general.spec.js` around lines 293 - 338, Restore
test global state through hooks rather than test-body cleanup: in
shepherd.js/test/unit/utils/general.spec.js:293-338, add an afterEach hook for
the waitForAttachToElement() describe block that calls vi.unstubAllGlobals(),
and remove both trailing calls; in shepherd.js/test/unit/tour.spec.js:1030-1064,
move the console.error spy restoration into an afterEach hook or use
vi.restoreAllMocks() there.

Fix all unresolved CodeRabbit comments on this PR:

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