fix: resolve tour lifecycle races in beforeShowPromise, start, and confirmCancel

.claude/skills/verify-tour-lifecycle/SKILL.md

@@ -0,0 +1,115 @@
+---
+name: verify-tour-lifecycle
+description: Exhaustively check Shepherd's tour and step lifecycle against every order of events up to a bound. Use when changing tour or step show, hide, start, cancel, complete, or destroy logic, beforeShowPromise, waitForElement, confirmCancel, or modal overlay setup and teardown in shepherd.js, or when asked to verify tour races or state.
+---
+
+# Verify the tour lifecycle
+
+Ordinary tests check the event orders someone thought to write. This check replays **every** order of events up to 6 events, from a fresh tour, against the real `src/tour.ts` and `src/step.ts`, and checks the page after each event.
+
+## What it promises
+
+- **ScreenMatchesTour.** Once events settle, a running tour shows at most its current step's popup and exactly one overlay, and an ended tour leaves no popup, overlay, or target marking. *Harm if broken: a popup for the wrong step, or a page left dimmed with no way to dismiss it.*
+- **SingleEnding.** A run ends once, and a cancel only ends the run that confirmed it. *Harm if broken: `cancel` handlers run twice, or a late confirmation kills a tour the user restarted.*
+- **NoThrow.** No lifecycle call rejects.
+
+## Does your change touch it?
+
+Run the check if you changed any of these. The check was last confirmed to describe them at the sync commit below.
+
+| File | Covered code |
+| --- | --- |
+| `shepherd.js/src/tour.ts` | `start`, `next`, `back`, `show`, `_showStep`, `cancel`, `complete`, `_done`, `_setupActiveTour`, `setupModal`, `_updateStateBeforeShow` |
+| `shepherd.js/src/step.ts` | `show`, `_show`, `hide`, `destroy`, `_setupElements`, `_teardownElements`, `_updateStepTargetOnHide` |
+| `shepherd.js/src/utils/general.ts` | `waitForAttachToElement`, `resolveAttachToElement` |
+| `shepherd.js/src/components/shepherd-modal.ts` | `hide`, `show`, `destroy`, `setupForStep` |
+
+**Sync commit:** `cba285a53dd85d87c185ee6b04070f87b834b14b`
+
+To see what changed since then, run this from the repo root:
+
+```sh
+git log --oneline cba285a53dd85d87c185ee6b04070f87b834b14b..HEAD -- shepherd.js/src/tour.ts shepherd.js/src/step.ts shepherd.js/src/utils/general.ts shepherd.js/src/components/shepherd-modal.ts
+```
+
+Move the sync commit forward only after you confirm the scenarios and this table still describe the code at the new commit. A passing run alone does not confirm that.
+
+## Run it
+
+From `shepherd.js/`, using the binary directly because `pnpm exec` triggers a full package build:
+
+```sh
+./node_modules/.bin/vitest run --config test/verify/vitest.config.mjs   # about 15 seconds
+node test/verify/run-mutants.mjs                                        # about 3 minutes
+```
+
+Vitest is pinned by the repo's lockfile. `pnpm test:unit` and CI do not run this check, because the filename is `*.verify.js`.
+
+Run the mutants too when you change the check or the guards they remove. Each one removes a guard in memory, leaving the files on disk alone, and every one must print `caught`. `STALE` means the source text it targets has moved, so update `test/verify/mutants.mjs`. `SURVIVED` means the check has gone blind to that guard, or the mutant's text now matches a different line first, because only the first match in the file is replaced. The runner runs the unmutated check first and stops if that fails, since every mutant would then look caught.
+
+## Read the result
+
+A pass means the findings are **exactly** those recorded in `test/verify/baseline.json`, which is empty at the sync commit. Check that file before treating a pass as "no findings". The run prints every finding and one shortest trace for it, such as:
+
+```
+[beforeShowPromise / ScreenMatchesTour: popup "gated" visible after the tour ended] 34 trace(s)
+  e.g. tour.start() > tour.next() > tour.cancel() > resolve-beforeShowPromise#0(run 1)
+```
+
+Read the trace as a story: the tour starts, the user moves to the gated step, the tour is cancelled, and then the step's `beforeShowPromise` resolves.
+
+| What failed | What it means | What to do |
+| --- | --- | --- |
+| A test under `harness reaches the behaviour it claims to check` | Your change broke the harness or basic behaviour | Fix this first. The exhaustive result can't be trusted until it passes. |
+| `matches the recorded findings`, with a new key or a higher count | Most likely a regression | Follow the trace in the code. If the trace can happen in a real app, it is a bug in your change. |
+| `matches the recorded findings`, with a key gone or a lower count | A finding may be fixed, or a scenario may no longer reach it | Replay the old trace to see which. |
+
+It's a modeling error, not a product bug, only if the trace can't happen in a real app, or if the harness's fake promise or timer behaves differently from the real one. Say which and why, then fix the harness. Never change a property to make a real trace pass.
+
+Fixing a finding lets exploration continue past where it used to stop, so other counts can rise and new keys can appear. Read each one before accepting it.
+
+### Update the baseline
+
+Only after you've read every changed key and decided the new behaviour is right:
+
+```sh
+VERIFY_RECORD=1 ./node_modules/.bin/vitest run --config test/verify/vitest.config.mjs
+```
+
+Recording to make a regression pass hides the bug. Put the reason for any change to `baseline.json` in the commit message.
+
+To look deeper once, run this without recording. It takes about 2 minutes and needs the larger heap, because the run exhausts Node's default limit. The baseline has no entry for depth 7, so any finding fails the run. Depth 7 found nothing at the sync commit.
+
+```sh
+NODE_OPTIONS=--max-old-space-size=8192 VERIFY_DEPTH=7 ./node_modules/.bin/vitest run --config test/verify/vitest.config.mjs
+```
+
+## Guards this check protects
+
+There are no open findings at the sync commit, so `baseline.json` is empty and any finding is new. These guards closed earlier findings:
+
+- **`_showGeneration`** in `Tour` and `Step.show`. A pending `waitForElement` or `beforeShowPromise` does nothing if a newer `show()` happened or the tour ended. Without it, `tour.start() > tour.next() > tour.cancel() > resolve` brings back the ended tour's popup and dimmed overlay. Every way a wait can go stale must bump this counter, because `Tour.show` has no separate `isActive()` check.
+- **Restart teardown** in `Tour.start` and `Tour.setupModal`. Starting a running tour hides the current step and destroys the old modal. Without it, `tour.start() > tour.start() > tour.cancel()` leaves the page dimmed.
+- **`_runGeneration`** in `Tour.cancel`. A `confirmCancel` answer does nothing if the run that asked has ended or restarted. Without it, `tour.start() > tour.cancel() > tour.complete() > tour.start() > answer-yes` ends the new run.
+
+## What this does not cover
+
+Only the `lifecycle edge cases` unit tests in `test/unit/tour.spec.js` protect `tour.hide()`, `removeStep`, repeated `step.show()`, a second tour, and handlers that end a restart. They check specific orders, not every order.
+
+- **Bound.** Orders of at most 6 events, in tours of 2 or 3 steps with one asynchronous step, first or second. A longer sequence or a larger tour is not checked.
+- **Events left out.** `tour.show(id)`, `tour.hide()`, `removeStep`, `updateStepOptions`, `showOn`, `skipMissingElement`, `advanceOn`, keyboard handling, and direct `step.*` calls. Removing a step that is waiting on `waitForElement` while it isn't the current step is known not to cancel that wait.
+- **One tour at a time.** A second `Tour` started during the first is not modeled.
+- **Timing granularity.** Promise callbacks settle fully between events, so each event's follow-up work is treated as one step. Two promises resolving in the same tick are not interleaved with each other's callbacks.
+- **Environment.** The DOM is happy-dom, with timers and `requestAnimationFrame` faked. This checks lifecycle logic only, not positioning, scrolling, CSS, focus restoration, or real browser behaviour.
+- **User handlers.** Event handlers and step callbacks that themselves call tour methods are not modeled.
+
+## Files
+
+All of these are in `shepherd.js/test/verify/`:
+
+- `tour-lifecycle.verify.js` holds the scenarios, the properties (`checkScreen`, and the listeners in `newWorld`), and the explorer.
+- `baseline.json` holds the recorded finding counts.
+- `mutants.mjs` and `run-mutants.mjs` hold and run the broken variants.
+- `vitest.config.mjs` holds the config and applies the mutants.
+
+To cover new behaviour, add a scenario to `SCENARIOS`, add a mutant that proves it can fail, and add to the coverage table above.

shepherd.js/src/step.ts

@@ -430,6 +430,7 @@ export class Step extends Evented {
   _resolvedAttachTo: StepOptionsAttachTo | null;
   _resolvedExtraHighlightElements?: HTMLElement[];
   _originalTabIndexes: Map<Element, string>;
+  _showRequest = 0;
   classPrefix?: string;
   declare cleanup: Function | null;
   el?: HTMLElement | null;
@@ -590,13 +591,25 @@ export class Step extends Evented {
   }
 
   /**
-   * Wraps `_show` and ensures `beforeShowPromise` resolves before calling show
+   * Wraps `_show` and ensures `beforeShowPromise` resolves before calling show.
+   * The step is not shown if the tour moved on or ended while it was pending.
    */
   show() {
     if (isFunction(this.options.beforeShowPromise)) {
-      return Promise.resolve(this.options.beforeShowPromise()).then(() =>
-        this._show()
-      );
+      // Read before calling `beforeShowPromise`, since a newer `tour.show()` or
+      // the tour ending while it is pending must keep this show from rendering.
+      const generation = this.tour._showGeneration;
+      // Direct `step.show()` calls don't change the tour's generation.
+      const request = ++this._showRequest;
+
+      return Promise.resolve(this.options.beforeShowPromise()).then(() => {
+        if (
+          generation === this.tour._showGeneration &&
+          request === this._showRequest
+        ) {
+          this._show();
+        }
+      });
     }
     return Promise.resolve(this._show());
   }

shepherd.js/src/tour.ts

@@ -123,12 +123,19 @@ export class Tour extends Evented {
   steps: Array<Step>;
 
   /**
-   * Monotonically increasing id of `show()` calls, used to invalidate pending
-   * `waitForElement` waits that were superseded by a newer `show()`.
+   * Monotonically increasing id of `show()` calls and tour endings, used to
+   * invalidate pending `waitForElement` and `beforeShowPromise` waits.
    * @private
    */
   _showGeneration = 0;
 
+  /**
+   * Changes whenever a run of the tour begins or ends, used to invalidate
+   * pending `confirmCancel` answers that belong to an earlier run.
+   * @private
+   */
+  _runGeneration = 0;
+
   constructor(options: TourOptions = {}) {
     super();
 
@@ -229,7 +236,13 @@ export class Tour extends Evented {
       let stopTour;
 
       if (isFunction(this.options.confirmCancel)) {
+        const run = this._runGeneration;
         stopTour = await this.options.confirmCancel();
+
+        // The answer only applies to the run that asked for it.
+        if (run !== this._runGeneration) {
+          return;
+        }
       } else {
         stopTour = window.confirm(cancelMessage);
       }
@@ -271,6 +284,9 @@ export class Tour extends Evented {
    * Hide the current step
    */
   hide() {
+    // A show still waiting on a promise or element must not undo this hide.
+    this._showGeneration++;
+
     const currentStep = this.getCurrentStep();
 
     if (currentStep) {
@@ -321,6 +337,8 @@ export class Tour extends Evented {
     });
 
     if (current && current.id === name) {
+      // The removed step may still be waiting on its `beforeShowPromise`.
+      this._showGeneration++;
       this.currentStep = undefined;
 
       // If we have steps left, show the first one, otherwise just cancel the tour
@@ -344,7 +362,7 @@ export class Tour extends Evented {
 
     this._updateStateBeforeShow();
 
-    // Invalidate any pending `waitForElement` wait from a previous call.
+    // Invalidate any pending wait from a previous call.
     const generation = ++this._showGeneration;
 
     const shouldSkipStep =
@@ -376,9 +394,9 @@ export class Tour extends Evented {
     ) {
       if (waitTimeout > 0) {
         return waitForAttachToElement(step, waitTimeout).then((element) => {
-          // A newer `show()`, or cancelling/completing the tour, supersedes
-          // this pending wait.
-          if (generation !== this._showGeneration || !this.isActive()) {
+          // A newer `show()`, the tour ending or hiding, or another tour
+          // starting, supersedes this pending wait.
+          if (generation !== this._showGeneration) {
             return;
           }
 
@@ -423,8 +441,20 @@ export class Tour extends Evented {
   async start() {
     this.trigger('start');
 
-    // Save the focused element before the tour opens
-    this.focusedElBeforeOpen = document.activeElement as HTMLElement | null;
+    if (this.isActive()) {
+      // Restarting a running tour. Focus may now be inside its own popup, so
+      // keep the element saved by the original start.
+      const run = this._runGeneration;
+      this.currentStep?.hide();
+
+      // A `hide` handler may have ended the tour, which must stay ended.
+      if (run !== this._runGeneration) {
+        return;
+      }
+    } else {
+      // Save the focused element before the tour opens
+      this.focusedElBeforeOpen = document.activeElement as HTMLElement | null;
+    }
 
     this.currentStep = null;
 
@@ -440,6 +470,11 @@ export class Tour extends Evented {
    * @private
    */
   _done(event: string) {
+    // Invalidate any pending `beforeShowPromise`, which could otherwise bring
+    // back the popup and modal of the ended tour.
+    this._showGeneration++;
+    this._runGeneration++;
+
     const index = this.steps.indexOf(this.currentStep as Step);
     if (Array.isArray(this.steps)) {
       this.steps.forEach((step) => step.destroy());
@@ -449,7 +484,10 @@ export class Tour extends Evented {
 
     this.trigger(event, { index });
 
-    Shepherd.activeTour = null;
+    // Another tour may have started while this one waited on `confirmCancel`.
+    if (this.isActive()) {
+      Shepherd.activeTour = null;
+    }
     this.trigger('inactive', { tour: this });
 
     if (this.modal) {
@@ -473,6 +511,13 @@ export class Tour extends Evented {
    * Make this tour "active"
    */
   _setupActiveTour() {
+    this._runGeneration++;
+
+    // Taking over from another tour cancels whatever that tour was waiting on.
+    if (Shepherd.activeTour && !this.isActive()) {
+      Shepherd.activeTour._showGeneration++;
+    }
+
     this.trigger('active', { tour: this });
 
     Shepherd.activeTour = this;
@@ -483,7 +528,13 @@ export class Tour extends Evented {
    */
   setupModal() {
     const container = this.options.modalContainer || document.body;
+    this.modal?.destroy();
     this.modal = createShepherdModal(container);
+
+    // A step that is already open keeps its dimming and highlight.
+    if (this.currentStep?.isOpen()) {
+      this.modal.setupForStep(this.currentStep);
+    }
   }
 
   /**