Skip to content

feat: Add guided tour component library using react-joyride v3#1226

Open
YoavLev wants to merge 3 commits intocadence-workflow:masterfrom
YoavLev:yoav.levy/react-joyride-tour
Open

feat: Add guided tour component library using react-joyride v3#1226
YoavLev wants to merge 3 commits intocadence-workflow:masterfrom
YoavLev:yoav.levy/react-joyride-tour

Conversation

@YoavLev
Copy link
Copy Markdown
Contributor

@YoavLev YoavLev commented Mar 31, 2026

Summary

  • Adds reusable guided tour components (GuidedTourProvider, GuidedTourTooltip) built on react-joyride v3 with BaseUI styling
  • Includes localStorage-based completion tracking per tour ID, supporting multiple independent tours per page
  • No pages are wired up — this PR ships the component library so tours can be added to any page
  • Includes GUIDED_TOUR.md with usage docs, code examples, and targeting strategies

Components

Component Purpose
GuidedTourProvider Wraps useJoyride hook, manages auto-start on first visit, tracks completion per tourId in localStorage
GuidedTourTooltip Custom tooltip styled with BaseUI (Header, Body, Footer with progress and navigation)

Test plan

  • npm run typecheck — no type errors
  • npm run lint — no lint errors
  • 15 unit tests passing (7 provider + 8 tooltip)

🤖 Generated with Claude Code

Screen.Recording.2026-03-24.at.11.42.01.mov

Comment on lines +22 to +31
<Button
size="mini"
kind="tertiary"
shape="square"
aria-label="Close tour"
onClick={() => controls.skip()}
overrides={overrides.closeButton}
>
<MdClose size={14} />
</Button>
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Quality: Tooltip buttons don't spread joyride's built-in a11y props

The custom tooltip calls controls.skip(), controls.next(), controls.prev() directly on click handlers instead of spreading backProps, primaryProps, closeProps, and skipProps from TooltipRenderProps. The react-joyride reference docs explicitly state: "Always spread button props on their respective buttons. This ensures correct accessibility attributes and action handling."

By bypassing these props, the buttons lose aria-label, data-action, role, and title attributes that joyride provides for accessibility and internal event tracking. This also means joyride's internal event flow (which triggers step:after hooks, analytics, etc.) may not fire correctly for button interactions.

Suggested fix:

<Button
  {...closeProps}
  size="mini"
  kind="tertiary"
  shape="square"
  overrides={overrides.closeButton}
>
  <MdClose size={14} />
</Button>
...
<Button {...backProps} size="compact" kind="secondary">Back</Button>
<Button {...primaryProps} size="compact" kind="primary">
  {isLastStep ? 'Done' : 'Next'}
</Button>

Was this helpful? React with 👍 / 👎 | Reply gitar fix to apply this suggestion

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Does this applicable to te react-joyride version 3?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Yes, this is still applicable in react-joyride v3. The TooltipRenderProps type in v3 includes the same backProps, primaryProps, closeProps, and skipProps — each carrying aria-label, data-action, onClick, role, and title attributes.

Details

Looking at the code, guided-tour-tooltip.types.ts already sets Props = TooltipRenderProps, so those props are available. However, the component only destructures controls, index, isLastStep, size, step, and tooltipProps — and calls controls.skip(), controls.next(), controls.prev() directly on the button click handlers.

Using controls.* bypasses joyride's own button props, which means:

  • The buttons miss the aria-label, data-action, role, and title attributes joyride sets on backProps/primaryProps/skipProps/closeProps
  • Joyride's internal event tracking (e.g. step:after hooks) may not fire as expected

The fix would be to destructure and use the onClick from those props (e.g. backProps.onClick, primaryProps.onClick, skipProps.onClick) instead of controls.prev(), controls.next(), controls.skip() — or spread the full prop objects onto the respective buttons.

Comment on lines +16 to +18
export const GuidedTourContext = createContext<GuidedTourContextType>(
{} as GuidedTourContextType
);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Edge Case: Context default value will crash if used outside provider

The context is initialized with {} as GuidedTourContextType, so any component calling useContext(GuidedTourContext) outside a GuidedTourProvider will get an empty object. Accessing controls.start() will throw TypeError: Cannot read properties of undefined. Consider either providing a meaningful error or creating a useGuidedTour() hook that throws a descriptive error when the provider is missing.

Suggested fix:

export function useGuidedTour() {
  const ctx = useContext(GuidedTourContext);
  if (!ctx.controls) {
    throw new Error('useGuidedTour must be used within a GuidedTourProvider');
  }
  return ctx;
}

Was this helpful? React with 👍 / 👎 | Reply gitar fix to apply this suggestion

Comment on lines +137 to +142
```tsx
<GuidedTourProvider tourId="my-page-overview" steps={overviewSteps}>
<GuidedTourProvider tourId="my-page-new-feature" steps={featureSteps}>
{children}
</GuidedTourProvider>
</GuidedTourProvider>
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Edge Case: Nesting providers shadows outer tour's context

GUIDED_TOUR.md documents nesting multiple GuidedTourProvider components for multiple tours on one page, but they all use the same GuidedTourContext. The inner provider will shadow the outer one's controls, making it impossible for children to access the outer tour's controls via useContext. This is fine if children only need the innermost tour, but the docs don't mention this limitation. Consider documenting it or using per-tour context instances.

Was this helpful? React with 👍 / 👎 | Reply gitar fix to apply this suggestion

Add reusable guided tour components for creating product tours with
BaseUI-styled tooltips and localStorage-based completion tracking.

Components:
- GuidedTourProvider: wraps useJoyride hook, manages auto-start on
  first visit, tracks completion per tourId in localStorage
- GuidedTourTooltip: custom tooltip with BaseUI styling (Header,
  Body, Footer with progress and navigation)

Includes GUIDED_TOUR.md with usage docs and code examples.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Yoav Levy <yoavlev2@gmail.com>
@YoavLev YoavLev force-pushed the yoav.levy/react-joyride-tour branch from d6929c3 to 4c16853 Compare March 31, 2026 13:24
YoavLev and others added 2 commits March 31, 2026 15:33
Add reusable guided tour components for creating product tours with
BaseUI-styled tooltips and localStorage-based completion tracking.

Components:
- GuidedTourProvider: wraps useJoyride hook, manages auto-start on
  first visit, tracks completion per tourId in localStorage
- GuidedTourTooltip: custom tooltip with BaseUI styling (Header,
  Body, Footer with progress and navigation)

Includes GUIDED_TOUR.md with usage docs and code examples.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Yoav Levy <yoavlev2@gmail.com>
@YoavLev YoavLev force-pushed the yoav.levy/react-joyride-tour branch from 275d571 to 01b07ee Compare April 1, 2026 10:05
@gitar-bot
Copy link
Copy Markdown

gitar-bot bot commented Apr 1, 2026

Code Review ⚠️ Changes requested 1 resolved / 5 findings

Introduces a guided tour component library using react-joyride v3, but has critical accessibility issues where tooltip buttons don't spread joyride's built-in a11y props, unsafe context initialization that crashes outside providers, and config errors causing permanent loading skeleton states.

⚠️ Quality: Tooltip buttons don't spread joyride's built-in a11y props

📄 src/components/guided-tour/guided-tour-tooltip/guided-tour-tooltip.tsx:22-31 📄 src/components/guided-tour/guided-tour-tooltip/guided-tour-tooltip.tsx:39-49

The custom tooltip calls controls.skip(), controls.next(), controls.prev() directly on click handlers instead of spreading backProps, primaryProps, closeProps, and skipProps from TooltipRenderProps. The react-joyride reference docs explicitly state: "Always spread button props on their respective buttons. This ensures correct accessibility attributes and action handling."

By bypassing these props, the buttons lose aria-label, data-action, role, and title attributes that joyride provides for accessibility and internal event tracking. This also means joyride's internal event flow (which triggers step:after hooks, analytics, etc.) may not fire correctly for button interactions.

Suggested fix
<Button
  {...closeProps}
  size="mini"
  kind="tertiary"
  shape="square"
  overrides={overrides.closeButton}
>
  <MdClose size={14} />
</Button>
...
<Button {...backProps} size="compact" kind="secondary">Back</Button>
<Button {...primaryProps} size="compact" kind="primary">
  {isLastStep ? 'Done' : 'Next'}
</Button>
💡 Edge Case: Context default value will crash if used outside provider

📄 src/components/guided-tour/guided-tour-provider/guided-tour-provider.tsx:16-18

The context is initialized with {} as GuidedTourContextType, so any component calling useContext(GuidedTourContext) outside a GuidedTourProvider will get an empty object. Accessing controls.start() will throw TypeError: Cannot read properties of undefined. Consider either providing a meaningful error or creating a useGuidedTour() hook that throws a descriptive error when the provider is missing.

Suggested fix
export function useGuidedTour() {
  const ctx = useContext(GuidedTourContext);
  if (!ctx.controls) {
    throw new Error('useGuidedTour must be used within a GuidedTourProvider');
  }
  return ctx;
}
💡 Edge Case: Nesting providers shadows outer tour's context

📄 src/components/guided-tour/GUIDED_TOUR.md:137-142 📄 src/components/guided-tour/guided-tour-provider/guided-tour-provider.tsx:16-18 📄 src/components/guided-tour/guided-tour-provider/guided-tour-provider.tsx:16 📄 src/components/guided-tour/guided-tour-provider/guided-tour-provider.tsx:61 📄 src/components/guided-tour/guided-tour-provider/guided-tour-provider.tsx:60-64

GUIDED_TOUR.md documents nesting multiple GuidedTourProvider components for multiple tours on one page, but they all use the same GuidedTourContext. The inner provider will shadow the outer one's controls, making it impossible for children to access the outer tour's controls via useContext. This is fine if children only need the innermost tour, but the docs don't mention this limitation. Consider documenting it or using per-tour context instances.

💡 Edge Case: Config error permanently shows loading skeleton on actions button

When useConfigValue('WORKFLOW_ACTIONS_ENABLED') fails (after react-query's default 3 retries), the dropdown button stays in a perpetual loading/skeleton state because isLoading={isActionsEnabledLoading || isActionsEnabledError}. Users can never open the actions dropdown if the config endpoint is unreachable. This follows the pre-existing pattern from DomainPageStartWorkflowButton, but since this is a new component it's worth considering a better UX—e.g., showing the dropdown in a degraded state (all actions disabled) or an error indicator with a retry option.

Suggested fix
Consider rendering the button in a usable but degraded state on error:

isLoading={isActionsEnabledLoading}
isError={isActionsEnabledError}

Then disable all actions when isError is true, so the user at least sees the dropdown exists and can potentially retry.
✅ 1 resolved
Quality: Pinned to react-joyride RC pre-release version

📄 package.json:69
react-joyride: 3.0.0-rc.3 is a release candidate. RC versions may have breaking changes before the stable 3.0.0 release, and pinning to an exact RC means no patch fixes will be picked up automatically. This is acceptable for now but should be tracked to upgrade to stable once available.

🤖 Prompt for agents
Code Review: Introduces a guided tour component library using react-joyride v3, but has critical accessibility issues where tooltip buttons don't spread joyride's built-in a11y props, unsafe context initialization that crashes outside providers, and config errors causing permanent loading skeleton states.

1. ⚠️ Quality: Tooltip buttons don't spread joyride's built-in a11y props
   Files: src/components/guided-tour/guided-tour-tooltip/guided-tour-tooltip.tsx:22-31, src/components/guided-tour/guided-tour-tooltip/guided-tour-tooltip.tsx:39-49

   The custom tooltip calls `controls.skip()`, `controls.next()`, `controls.prev()` directly on click handlers instead of spreading `backProps`, `primaryProps`, `closeProps`, and `skipProps` from `TooltipRenderProps`. The react-joyride reference docs explicitly state: "Always spread button props on their respective buttons. This ensures correct accessibility attributes and action handling."
   
   By bypassing these props, the buttons lose `aria-label`, `data-action`, `role`, and `title` attributes that joyride provides for accessibility and internal event tracking. This also means joyride's internal event flow (which triggers `step:after` hooks, analytics, etc.) may not fire correctly for button interactions.

   Suggested fix:
   <Button
     {...closeProps}
     size="mini"
     kind="tertiary"
     shape="square"
     overrides={overrides.closeButton}
   >
     <MdClose size={14} />
   </Button>
   ...
   <Button {...backProps} size="compact" kind="secondary">Back</Button>
   <Button {...primaryProps} size="compact" kind="primary">
     {isLastStep ? 'Done' : 'Next'}
   </Button>

2. 💡 Edge Case: Context default value will crash if used outside provider
   Files: src/components/guided-tour/guided-tour-provider/guided-tour-provider.tsx:16-18

   The context is initialized with `{} as GuidedTourContextType`, so any component calling `useContext(GuidedTourContext)` outside a `GuidedTourProvider` will get an empty object. Accessing `controls.start()` will throw `TypeError: Cannot read properties of undefined`. Consider either providing a meaningful error or creating a `useGuidedTour()` hook that throws a descriptive error when the provider is missing.

   Suggested fix:
   export function useGuidedTour() {
     const ctx = useContext(GuidedTourContext);
     if (!ctx.controls) {
       throw new Error('useGuidedTour must be used within a GuidedTourProvider');
     }
     return ctx;
   }

3. 💡 Edge Case: Nesting providers shadows outer tour's context
   Files: src/components/guided-tour/GUIDED_TOUR.md:137-142, src/components/guided-tour/guided-tour-provider/guided-tour-provider.tsx:16-18, src/components/guided-tour/guided-tour-provider/guided-tour-provider.tsx:16, src/components/guided-tour/guided-tour-provider/guided-tour-provider.tsx:61, src/components/guided-tour/guided-tour-provider/guided-tour-provider.tsx:60-64

   GUIDED_TOUR.md documents nesting multiple `GuidedTourProvider` components for multiple tours on one page, but they all use the same `GuidedTourContext`. The inner provider will shadow the outer one's `controls`, making it impossible for children to access the outer tour's controls via `useContext`. This is fine if children only need the innermost tour, but the docs don't mention this limitation. Consider documenting it or using per-tour context instances.

4. 💡 Edge Case: Config error permanently shows loading skeleton on actions button

   When `useConfigValue('WORKFLOW_ACTIONS_ENABLED')` fails (after react-query's default 3 retries), the dropdown button stays in a perpetual loading/skeleton state because `isLoading={isActionsEnabledLoading || isActionsEnabledError}`. Users can never open the actions dropdown if the config endpoint is unreachable. This follows the pre-existing pattern from `DomainPageStartWorkflowButton`, but since this is a new component it's worth considering a better UX—e.g., showing the dropdown in a degraded state (all actions disabled) or an error indicator with a retry option.

   Suggested fix:
   Consider rendering the button in a usable but degraded state on error:
   
   isLoading={isActionsEnabledLoading}
   isError={isActionsEnabledError}
   
   Then disable all actions when isError is true, so the user at least sees the dropdown exists and can potentially retry.

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

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