Skip to content

improvement(tour): fix tour auto-start logic and standardize selectors#3751

Merged
waleedlatif1 merged 2 commits intostagingfrom
waleedlatif1/hide-tour
Mar 25, 2026
Merged

improvement(tour): fix tour auto-start logic and standardize selectors#3751
waleedlatif1 merged 2 commits intostagingfrom
waleedlatif1/hide-tour

Conversation

@waleedlatif1
Copy link
Collaborator

Summary

  • Fix tour opening at random times by using module-level auto-start guard that survives component remounts
  • Use disabledRef so auto-start effect doesn't re-trigger on navigation
  • Cancel in-flight transitions and rAF callbacks when tour is paused or stopped
  • Fix unsafe ref forwarding in TourTooltipAdapter and Back button a11y
  • Stabilize virtualRef in TourTooltip to avoid unnecessary Radix repositioning
  • Standardize all tour step selectors to use data-tour attributes
  • Add loop prop to Tooltip.Preview (defaults to true), use loop={false} for auto-connect
  • Move tooltip trigger to info icon instead of label text for settings with previews
  • Fix preview styling to be flush with tooltip edges and match border radius
  • Replace auto-connect-on-drop video

Type of Change

  • Bug fix
  • Improvement

Testing

Tested manually

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

@vercel
Copy link

vercel bot commented Mar 25, 2026

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

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped Mar 25, 2026 1:19am

Request Review

@cursor
Copy link

cursor bot commented Mar 25, 2026

PR Summary

Medium Risk
Changes tour start/transition timing and selector targeting across sidebar/workflow tabs, which could cause tours to not appear or to anchor incorrectly if any data-tour attributes are missing or mismatched. No security-sensitive logic is affected.

Overview
Improves product tour reliability by preventing repeated auto-starts across component remounts (module-level guard), honoring disabled via a ref, and cancelling pending timers/rAF work when tours pause/stop to avoid late tooltip reveals.

Standardizes tour targeting to data-tour selectors across navigation and workflow panel tabs (adding the attributes in sidebar.tsx/panel.tsx and updating step definitions), and hardens tooltip rendering by safely forwarding Joyride refs, stabilizing Radix virtualRef, and making the Back button non-focusable/hidden on the first step.

Updates settings tooltips to trigger from an info icon, tweaks Tooltip.Preview styling, and adds a loop prop (used to disable looping for the auto-connect preview).

Written by Cursor Bugbot for commit 7fddadf. Configure here.

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 25, 2026

Greptile Summary

This PR makes several targeted fixes and improvements to the product tour system. The primary change — a module-level autoStartAttempted Set — correctly prevents the tour from re-firing on component remounts (e.g. navigating between workflows) while still resetting on full page reload. Supporting changes include a disabledRef to safely read disabled inside async timer callbacks, shared scheduleReveal/cancelPendingTransitions helpers that clean up in-flight RAF and timer state when the tour is paused or stopped, and a fix to the virtualRef passed to Radix Popover so repositioning isn't triggered on every render.

Key changes across the PR:

  • use-tour.ts: Module-level autoStartAttempted guard; disabledRef for stale-closure safety; scheduleReveal and cancelPendingTransitions extracted to eliminate duplicated double-rAF boilerplate; proper cleanup on unmount for both transition timers and retrigger timers.
  • tour-shared.tsx: setJoyrideRef callback now handles both callback-ref and RefObject forms from Joyride; Back button changes from opacity-0 pointer-events-none to invisible + tabIndex={-1}, which is semantically stronger (visibility:hidden removes the element from the accessibility tree entirely).
  • tour-tooltip.tsx: virtualRef stabilized with useRef so Radix Popover doesn't recompute position on every render of the parent.
  • tooltip.tsx: loop prop added to Preview (default true); negative margins (-mx-[8px] -mb-[3.5px]) make the preview flush with the tooltip's edges, correctly counteracting the parent's px-[8px] py-[3.5px] padding.
  • general.tsx: Tooltip trigger moved from the label text to an Info icon, improving UX and preserving the htmlFor association. loop={false} used for the auto-connect video.
  • Selector standardization: All tour step selectors migrated to data-tour attributes; existing data-item-id/data-tab-button attributes are preserved for backward compatibility.
  • One concern worth reviewing: autoStartAttempted.add(storageKey) is called inside the timer callback rather than before it, which means the guard can be bypassed if the effect re-runs before the delay elapses (see inline comment).

Confidence Score: 4/5

  • Safe to merge with one targeted fix recommended in use-tour.ts
  • The PR resolves real, well-described bugs (tour firing on remounts, stale closure in auto-start, ref churn, Radix repositioning noise) with clean, well-commented implementations. Prior review concerns have been addressed. One remaining issue — autoStartAttempted.add() being called inside the timer rather than before it — could allow the guard to be bypassed on rapid effect re-runs, but all call-site deps are effectively static constants so the risk in practice is very low. Everything else is solid.
  • apps/sim/app/workspace/[workspaceId]/components/product-tour/use-tour.ts — auto-start guard ordering

Important Files Changed

Filename Overview
apps/sim/app/workspace/[workspaceId]/components/product-tour/use-tour.ts Core logic refactor: adds module-level auto-start guard, disabledRef pattern, shared scheduleReveal/cancelPendingTransitions helpers, and proper RAF/timer cleanup. One minor ordering subtlety in transitionToStep (setIsTooltipVisible before cancelPendingTransitions), but safe in React's batched update model.
apps/sim/app/workspace/[workspaceId]/components/product-tour/tour-shared.tsx Improved ref forwarding in TourTooltipAdapter handles both callback and RefObject refs; useCallback memoization prevents null→node ref churn. Back button now uses invisible + tabIndex=-1 for correct a11y (visibility:hidden removes element from accessibility tree).
apps/sim/components/emcn/components/tour-tooltip/tour-tooltip.tsx virtualRef is now a stable useRef whose .current is updated synchronously, preventing Radix Popover from treating each render as a new anchor element and triggering unnecessary repositioning.
apps/sim/components/emcn/components/tooltip/tooltip.tsx Adds loop prop (defaults true) to Preview; negative margin trick (-mx-[8px] -mb-[3.5px]) makes preview flush with tooltip edges by canceling the parent's px-[8px] py-[3.5px] padding; rounded-b-[4px] matches parent border-radius.
apps/sim/app/workspace/[workspaceId]/settings/components/general/general.tsx Tooltip trigger moved from label text to Info icon — better UX and preserves the label's htmlFor association with the Switch. loop={false} applied to auto-connect video preview.

Sequence Diagram

sequenceDiagram
    participant C as Component Mount
    participant H as useTour Hook
    participant S as autoStartAttempted Set
    participant LS as localStorage
    participant RAF as rAF / setTimeout

    C->>H: mount (disabled=false)
    H->>S: has(storageKey)?
    S-->>H: false
    H->>LS: isTourCompleted(storageKey)?
    LS-->>H: false
    H->>RAF: setTimeout(autoStartDelay)

    Note over C,H: Component remounts (e.g. navigation)
    C->>H: unmount → cleanup clears timer
    C->>H: remount (disabled=false)
    H->>S: has(storageKey)?
    S-->>H: false (add() not called yet)
    H->>RAF: setTimeout(autoStartDelay) again ⚠️

    RAF-->>H: timer fires
    H->>H: check disabledRef.current
    H->>S: add(storageKey)
    H->>RAF: scheduleReveal() → rAF(outer) → rAF(inner) → setIsTooltipVisible(true)

    Note over H,RAF: On step transition
    H->>H: setIsTooltipVisible(false)
    H->>RAF: cancelPendingTransitions() → clears transitionTimer + rafRef
    H->>RAF: setTimeout(FADE_OUT_MS)
    RAF-->>H: fires → setStepIndex, scheduleReveal()

    Note over H,RAF: On pause/stop/unmount
    H->>RAF: cancelPendingTransitions() → cancelAnimationFrame(rafRef.current)
Loading

Reviews (2): Last reviewed commit: "fix(tour): address PR review comments" | Re-trigger Greptile

- Move autoStartAttempted.add() inside timer callback to prevent
  blocking auto-start when tour first mounts while disabled
- Memoize setJoyrideRef with useCallback to prevent ref churn
- Remove unused joyrideRef
@waleedlatif1
Copy link
Collaborator Author

@greptile

@waleedlatif1
Copy link
Collaborator Author

@cursor review

Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

@waleedlatif1 waleedlatif1 merged commit 96b171c into staging Mar 25, 2026
12 checks passed
@waleedlatif1 waleedlatif1 deleted the waleedlatif1/hide-tour branch March 25, 2026 01:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant