Skip to content

refactor: convert product-tours from Redux to React Query - #1968

Open
brian-smith-tcril wants to merge 1 commit into
bsmith/react-query-course-recommendationsfrom
bsmith/react-query-product-tours
Open

refactor: convert product-tours from Redux to React Query#1968
brian-smith-tcril wants to merge 1 commit into
bsmith/react-query-course-recommendationsfrom
bsmith/react-query-product-tours

Conversation

@brian-smith-tcril

@brian-smith-tcril brian-smith-tcril commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Summary

Converts product tours (the tours Redux slice) from Redux to React Query for server state and a React context for the client UI flags, per OEP-0067 ADR-0010. Part of the Redux → React Query migration (#1946), stacked on the course-recommendations pattern-setter (the branch below).

Behavior is unchanged — the new-user modal, the new-user / existing-user / courseware tours, and the launch button all trigger and dismiss exactly as before. Verified with the full test suite and a live manual smoke pass (details in the decision log).

What changed

  • React Query data layer: product-tours/data/queryKeys.ts (keyed by username) + data/apiHooks.tsuseTourData (useQuery) and useEndCourseHomeTour / useEndCoursewareTour (useMutation).
  • Client state → context: product-tours/TourContext.tsx mirrors the former slice's show-flags via useReducer; TourProvider is supplied by TabPage (the lowest node covering all tour consumers — the sr-only launch button lives directly in TabPage, outside LoadedTabPage).
  • Consumers rewired: ProductTours.jsx and LaunchCourseHomeTourButton.jsx read from the query + context instead of Redux.
  • Redux removed: delete data/{slice,thunks,index}.js; drop the tours reducer from store.ts and the test store.
  • Behavior parity with the old slice: invalidateQueries on the end-tour mutations keeps the query cache in sync with the PATCH (so a dismissed tour doesn't flash back on remount), and refetchOnWindowFocus: false prevents a focus refetch from reopening a tour mid-session. The only intentional deviation is dropping the in-memory replay-flag persistence (a Redux-only quirk that never survived a page reload) — see the decision log.

Testing

Automated: npm run types, npm run lint, npm run build, and the full npm test suite all pass (102 suites / 888 tests). Unit tests cover TourContext, the query/mutation hooks, and the tour interaction flows.

Manual smoke (dev). Tours are driven by the server-side UserTour row (/api/user_tours/v1/<username>). Set it via Django admin at /admin/user_tours/usertour/course_home_tour_status ∈ {show-new-user-tour, show-existing-user-tour, no-tour} and the show_courseware_tour checkbox. Finishing/dismissing a tour PATCHes it back, so re-arm between runs. Keep DevTools → Network filtered to user_tours.

  1. New-user tour — set show-new-user-tour → Home: modal appears → Begin tour starts the tour (no PATCH) → Skip for now dismisses it and shows the abandon prompt (PATCH no-tour); completing the tour PATCHes no-tour.
  2. Existing-user tour — set show-existing-user-tour → Home: tour renders directly (no modal).
  3. Courseware tour — set show_courseware_tour: true → open a unit: checkpoint anchors to the sequence-nav. Note: the anchor #courseware-sequence-navigation is rendered by SequenceNavigation, which sits behind SequenceNavigationSlot (empty by default), so it only appears when that slot is populated — pre-existing, unrelated to this PR (verified by injecting the default component into the slot).
  4. Launch button — with a no-tour row, Home shows the Launch tour button → click starts the tour.
  5. Behavior parity (the two RQ-default fixes):
    • Dismiss the modal, then navigate away and back → it does not flash back (invalidateQueries keeps the cache in sync with the PATCH).
    • Begin the tour, then refocus the window → no refetch and the modal does not reopen over the tour (refetchOnWindowFocus: false).
  6. Fetch gateGET /v1/<username> fires only on Home + Courseware (not Dates/Progress/Discussion; the discussion_tours endpoint is a separate, unrelated feature).

All of the above verified live. Full analysis and results are in the decision log below.

Decisions

Full decision log

Decisions — Redux → React Query: product tours

Working notes for this PR (part of the wider Redux → React Query migration,
#1946). Not checked in — referenced when opening the PR. Stacked on the
course-recommendations conversion (#1966).

Target selection

  • Next conversion = product-tours (the tours slice). Picked as the next
    self-contained Redux slice to remove after course recommendations.
    • Rejected bookmark (addBookmark/removeBookmark): it is a genuine
      Redux mutation, but its bookmarked / bookmarkedUpdateState state lives in
      the shared courseware units model and is read by UnitTitleSlot and the
      sequence-nav UnitButton, not just BookmarkButton. Converting it now would
      be a partial job — the useMutation would still have to dispatch
      updateModel into the still-Redux units store to keep those consumers in
      sync — coupling it to the courseware model that belongs to a later phase.
      Defer until courseware/models.
    • Rejected api-only leaves (enrollment-alert, celebration): they use no
      Redux, so converting them wouldn't remove any Redux (same reason we passed on
      preferences-unsubscribe for the read pattern-setter).
    • product-tours is self-contained: its own tours slice + data/api.js
      (getTourData / patchTourData), read only by ProductTours.jsx and
      LaunchCourseHomeTourButton.jsx. It only reads courseHomeMeta (a
      not-yet-converted slice — acceptable mid-migration, same as recommendations
      reading courseware.courseId) and never writes the shared models store.
      Removing it deletes a whole reducer, and it exercises the next patterns we
      need in one focused PR: useQuery (tour data), useMutation (patch/dismiss),
      and client-state → React context/local state (the UI on/off flags, per
      OEP-0067's "client state stays in React").

Stacked PR

Conversion plan

The tours slice state is a mix of server data and client UI state, so it
splits three ways (this is the OEP-0067 "server → React Query, client → React
state/context" split in miniature):

tours slice piece Kind Becomes
getTourData (fetchTourData thunk) server read useTourDatauseQuery
patchTourData (endCourseHomeTour/endCoursewareTour) server write useEndCourseHomeTour / useEndCoursewareTouruseMutation
showCoursewareTour, showExistingUserCourseHomeTour, showNewUserCourseHomeModal, showNewUserCourseHomeTour client UI flags (server-seeded, then client-mutated) TourContext (React context), seeded from the query
toursEnabled pure server read (never client-mutated; read only by LaunchCourseHomeTourButton) not in the context — read from useTourData

Why a context (not just local state)

showNewUserCourseHomeTour is a cross-component signal:
LaunchCourseHomeTourButton (rendered in outline-tab/widgets/CourseTools and
tab-page/TabPage) flips it, and ProductTours (rendered in
tab-page/LoadedTabPage) reacts. Different subtrees → shared client state needs a
context (the same "react context conversion" learner-dashboard did). useQuery
is deduped by key, so both components can call useTourData and share the cached
server result; only the live show/dismiss flags live in the context.

Files

  • New files are TypeScript (.ts/.tsx) — matches the references (authn/LD
    write new contexts as .tsx, api/hooks as .ts) and the direction of travel.
    Existing .jsx/.js files we only edit (ProductTours.jsx,
    LaunchCourseHomeTourButton.jsx, TabPage.jsx) stay as-is; converting them to
    TS is out of scope here.
  • New: data/queryKeys.ts (rooted at appId), data/apiHooks.ts
    (useTourData + the two end-tour mutations), and TourContext.tsx at the
    feature rootsrc/product-tours/TourContext.tsx (exports TourProvider +
    useTourState; holds the show flags + disableCourseHomeTour,
    disableCoursewareTour, closeNewUserCourseHomeModal, launchCourseHomeTour;
    seeds itself from the useTourData result).
    • Context placement decision: colocated in the feature, following authn
      (src/<feature>/…Context.tsx) rather than learner-dashboard's central
      src/data/context/ — consistent with the colocated data-layer choice from
      refactor: convert course recommendations to React Query #1966. Placed at the feature root (not a components/ subdir like authn) because
      product-tours keeps all its components flat at the feature root; and not
      under data/ (it's client state, not a fetch concern).
  • Keep: data/api.js (getTourData / patchTourData unchanged — the
    query/mutation fns; note getTourData already swallows 401/403/404 into
    { toursEnabled: false }, so the query resolves rather than errors).
  • Delete: data/slice.js, data/thunks.js; remove the tours reducer from
    src/store.ts and setupTest.js's initializeTestStore.
  • Rewire consumers: ProductTours.jsx, LaunchCourseHomeTourButton.jsx.

Provider placement (key design point)

TourProvider must wrap both ProductTours and every LaunchCourseHomeTourButton.
Their common ancestor is the tab pageTabPage.jsx renders both the sr-only
button and LoadedTabPage (which renders ProductTours and the tab content,
including the outline CourseTools button).

Decision: wrap TabPage's entire return (<>…</><TourProvider>…</TourProvider>),
including HeaderSlot/FooterSlot and the loading/failed branches, rather than
wrapping only the button + LoadedTabPage. The sr-only LaunchCourseHomeTourButton
sits in the ['loaded','denied'] block before HeaderSlot, so the provider has
to open before the header regardless; wrapping the whole return is cleaner than
restructuring the fragment to wrap two non-adjacent children. Header/footer don't
consume the context — harmless.

Client state: faithful slice → context mirror

Chose a faithful 1:1 mirror of the tours slice as TourContext (over the
leaner "minimal context + local state" option) — lowest-risk, mechanical, and
matches learner-dashboard's react-context conversion. TourContext uses
useReducer whose cases mirror the slice reducers exactly (setTourData,
disableCourseHomeTour, disableCoursewareTour, closeNewUserCourseHomeModal,
launchCourseHomeTour), seeded from the useTourData result. ProductTours
keeps its existing local is*Enabled layer unchanged (it just reads the context
flags instead of useSelector(state.tours)).

toursEnabled is read from the query, not the context. Unlike the show*
flags, toursEnabled is pure server data — never mutated by any disable*/launch
action — and it's read by only LaunchCourseHomeTourButton (not ProductTours).
So it stays in React Query rather than being mirrored into TourContext. The
button reads it via useTourData(username, /* enabled */ false) — an
observe-only subscriber that reads whatever ProductTours put in the query cache
(same key) without triggering its own fetch. This preserves the original's fetch
optimization exactly: only ProductTours fetches (on its guarded tabs), so on
non-outline courseHomeMeta tabs (dates/progress, where the sr-only button also
renders) the cache is empty → button hidden, same as today. Rejected having the
button enable its own fetch (would GET tour data on those tabs — a regression,
worsened by our bare client's lack of staleTime).

The original end-tour thunks did two things — persist (patchTourData) and
flip a client flag (dispatch(disable*)). That splits cleanly: patchTourData
persistence is unchanged, now the mutationFn of the end-tour mutations; the
client flag flip becomes a TourContext action. Both are called in the tour's
onEnd (mutate(username) + context.disable*()), rather than coupling the hide
into the mutation's onSuccess (which would force the data hook to consume the
context).

Modal action renamed disable…closeNewUserCourseHomeModal. The slice
called it disableNewUserCourseHomeModal, but the action only sets
showNewUserCourseHomeModal = false, and that flag is the modal's isOpen. So
it's a transient close, not a permanent disable — the "won't reopen"
persistence is a separate patch (endCourseHomeTour) called alongside it in
onDismiss (and notably not in onStartTour, which just closes the modal to
reveal the tour). close names the actual behavior; disable over-implied
permanence the action doesn't provide. The tour actions
(disableCourseHomeTour/disableCoursewareTour) keep disable for now. See
Tour reappearance: matching Redux persistence below for how refetchOnWindowFocus
and mutation cache-invalidation preserve the original show/hide behavior.

Thunk/action → new mapping

  • fetchTourData(username)useTourData(username, shouldFetchTourData()); the
    guard matches the original effect guard — see ProductTours readability below.
  • setTourData(data) → the context seeds its flags from the query result.
  • endCourseHomeTour(username)useEndCourseHomeTour().mutate() (PATCH
    course_home_tour_status: 'no-tour') + context.disableCourseHomeTour().
  • endCoursewareTour(username)useEndCoursewareTour().mutate() (PATCH
    show_courseware_tour: false) + context.disableCoursewareTour().
  • closeNewUserCourseHomeModal()context.closeNewUserCourseHomeModal().
  • launchCourseHomeTour()context.launchCourseHomeTour().
  • ProductTours keeps its existing local is*Enabled state and sendTrackEvent
    wiring; it just sources the seed flags from context (seeded by the query) and
    calls mutations + context actions instead of dispatching thunks.
  • LaunchCourseHomeTourButton: toursEnabled from useTourData; onClick
    context.launchCourseHomeTour(). courseId still from the courseHome slice
    and org via useModel('courseHomeMeta') — not-yet-converted reads, acceptable.

ProductTours readability (from review)

  • The fetch guard is a shouldFetchTourData() function with early returns, not a
    chain of named booleans.
    It has three independent reasons to bail — (1) not
    authenticated (the endpoint is per-user), (2) not on a tab that has a tour (only
    outline/courseware; avoid needless calls), (3) on the outline tab but the
    proctoring panel hasn't loaded, so the tour's target widget (weekly-goal) isn't
    in the DOM yet. We tried encoding each reason in a variable name and it stayed
    unclear ("resolved"/"ready"/"tour tab" didn't convey why), so each return false is a guard clause with a comment stating its reason. Called inline:
    useTourData(username, shouldFetchTourData()).
  • Renamed the base tab vars isCoursewareTab/isOutlineTab
    coursewareTabActive/outlineTabActive (the is…Tab prefix read awkwardly);
    updated their other uses (the show* → is*Enabled sync effects and the modal
    isOpen).
  • End-tour persist+hide extracted into local helpers. Each tour
    onEnd/onDismiss must persist (mutation) and hide (context) — two coupled
    lines. Extracted endCoursewareTour() / endCourseHomeTour() (each =
    mutation.mutate(username) + disable*(), closing over the in-scope username
    so call sites pass no args) so every call site is one line. Kept two named
    functions
    over a general endTour(type): the two branches share no logic
    (different mutation and different disable), so a discriminator would just be a
    stringly-typed switch needing a defensive else — worse than two clear names.

LaunchCourseHomeTourButton (from review)

  • toursEnabled now needs username; the old selector didn't. Redux's tours
    slice was a single global box — ProductTours's thunk wrote state.tours and
    this button read the same state.tours, no key involved. React Query's cache is
    keyed (tourQueryKeys.user(username)), so to read the entry ProductTours
    cached the button must reconstruct the same key, which requires username. Not
    new coupling — both components already get username from the same
    getAuthenticatedUser(); the per-key cache just makes the address explicit for a
    value that is genuinely per-user. This is the general shape of Redux-global →
    RQ-per-key: readers name the key in exchange for dedupe / staleness / per-user
    isolation.
  • Consolidated getAuthenticatedUser() to one top-level call. The original
    called it inside handleClick (only administrator, for the track event). The
    button now also needs username at render time for the observe-only query, so
    we destructure { administrator, username } = getAuthenticatedUser() || {} once
    at the top — auth is stable for the render, one call reads cleaner, and it
    matches how ProductTours.jsx sources auth.
  • useSelector stays for state.courseHome.courseId (feeds
    useModel('courseHomeMeta') for org) — a not-yet-converted slice, acceptable
    mid-migration.

Tour reappearance: matching Redux persistence

The tours slice was a single global, persisted store, and that persistence —
not any single line — is what governed when tours/modals reappeared. Redux had one
persistent store; React Query splits that persistence across two layers, and the
seam between them is where behavior can drift. We audited every reappearance path
and reduced it to three discriminators; each fix maps to exactly one.

Layer split. Server-derived flags (showNewUserCourseHomeModal,
showExistingUserCourseHomeTour, showCoursewareTour — all computed by
setTourData from server data) are persisted by the query cache, which (like
the old store) lives above the tab tree and survives SPA navigation. The one
purely-client flag showNewUserCourseHomeTour (set only by launchCourseHomeTour,
cleared only by disableCourseHomeTour) has no server field, so its
persistence depended on the Redux store's in-memory lifetime.

Note: the tour's step position was never persisted anywhere (the slice held
only booleans; position lives inside Paragon's ProductTour and dies on unmount).
So a tour never resumes mid-way in either Redux or RQ — on return it either
re-shows from the start or re-prompts. There is no "resume where you left off."

Discriminator 1 — did the action PATCH the server? If yes (finish/dismiss →
no-tour), the cache must match the patched server; a stale cache that still says
"show" is a bug (tour flashes back on the next remount). If no (start-from-modal
or abandon-mid-tour), the server still says "show," the cache correctly still says
"show," and re-showing on return is the intended re-prompt.
Fix: invalidateQueries in the end-tour mutations' onSuccess (the standard
RQ pattern; also the most faithful, since the original re-fetched fresh rather than
hand-patching local state). Keeps the cache in sync with the PATCH the way Redux's
dispatch(disable*) kept the store in sync. onStartTour deliberately fires no
mutation, so the intended re-prompt is untouched.

Discriminator 2 — remount (navigation) or no remount (focus)? On navigation the
component unmounts, local tour state is gone, and a re-derived modal is a clean
prompt. On focus nothing unmounts — the local tour is still running — so a
focus-triggered setTourData pops the modal on top of the live tour. The
original never refetched on focus (its fetch effect keyed on [proctoringPanelStatus]).
Fix: refetchOnWindowFocus: false on useTourData.

Discriminator 3 — is the "show" server-backed or client-only? Server-backed
persistence lives in the query cache, survives a page reload (re-fetched), and is
kept. The client-only showNewUserCourseHomeTour lived only in Redux memory: it
survived SPA tab-switches but died on any page reload — an in-memory artifact,
not durable state, and untested/undocumented. Its only observable effect was the
replay button re-launching the tour on every outline visit within one page load,
inconsistent with the server-driven tours (which re-prompt via the modal).
Decision: don't replicate it. Per-TabPage TourProvider resets client
flags on navigation, so the replay button is a clean one-shot per visit. We keep
every server-backed "show"; we drop only the in-memory replay-flag's survival
across a tab switch (which never survived a reload anyway).

The ProductTours.test.jsx suite only asserts single-mount behavior (server data →
correct tour on load; launch button → tour) and the 401/403/404 empty cases —
nothing about cross-navigation persistence — so both fixes and the Discriminator-3
drop leave every defined behavior intact.

Tests

Tour data still flows through the existing axios-mock-adapter setup (the same way
the rest of these suites mock course-metadata/outline/proctoring, all still Redux
until Phase 3) — useTourData (RQ) hits the same mocked tourDataUrl, so no
jest.mock of the hook/api is needed. The only structural change is supplying the
tour context.

  • Test renders wrap in TourProvider, mirroring TabPage. The provider lives
    in TabPage in production (it must — the sr-only LaunchCourseHomeTourButton is
    a direct child of TabPage, outside LoadedTabPage, so LoadedTabPage would
    be too low). These suites render LoadedTabPage/OutlineTab directly, bypassing
    TabPage, so each render is wrapped in <TourProvider> to reproduce that
    boundary. Chosen over mocking the tour components (ProductTours,
    LaunchCourseHomeTourButton): the wrap is one line, less fragile (a new tour
    consumer just works), and keeps the real components in the tree.
  • ProductTours.test.jsx: unchanged assertions; the Course Home Tours block
    wraps its LoadedTabPage render in TourProvider; the Courseware Tour block
    needs nothing (it renders CoursewareContainer → the real TabPage → provider).
  • Collateral (not tour tests): OutlineTab.test.jsx renders OutlineTab
    (→ CourseTools → the launch button) and, in two masquerade-banner tests,
    LoadedTabPage; ProgressTab.test.jsx renders LoadedTabPage in four tests.
    Those render sites are wrapped in TourProvider (progress tab never fetches —
    shouldFetchTourData is false off outline/courseware). Bare <ProgressTab/> /
    <OutlineTab/>-only renders with no tour consumer are left alone.

Verify

  • npm run types ✅ clean · npm run lint ✅ clean · npm run build ✅ clean
    (only the pre-existing webpack asset-size-limit warnings) · full suite ✅
    102 suites / 888 passed / 3 skipped / 0 failed. (Course.test's "displays
    learner tools … /previous/" is a pre-existing sequence-nav timing flake under
    full-suite load — passes in isolation and on re-run; unrelated to tours.)
  • Coverage: added TourContext.test.tsx and data/apiHooks.test.tsx (unit
    tests for the reducer/provider and the query/mutations) and extended
    ProductTours.test.jsx with the tour complete/dismiss/abandon flows + the
    streak/non-tour-tab guards, to bring patch coverage up to codecov's auto
    target. New-user completion advances the checkpoints via a recursive helper
    (not a while+await loop) to satisfy no-await-in-loop without a disable;
    interactions use userEvent.
  • git grep 'state.tours' → empty; tours reducer gone from store.ts and
    setupTest.js; data/{slice,thunks,index}.js deleted.
  • Collateral test fixes (consequences of ProductTours now needing a provider +
    a query client): OutlineTab/ProgressTab wrap their tab-content renders in
    TourProvider; DatesTab/DiscussionTab/CoursewareContainer (which build
    their own tree with RTL render) add a QueryClientProvider mirroring
    index.jsx.
  • Pre-existing (not from this PR): the courseware tour is dormant in stock
    config.
    It anchors to #courseware-sequence-navigation, which is rendered by
    SequenceNavigation — now behind SequenceNavigationSlot, an empty-by-default
    PluginSlot (Sequence.jsx:198). So the anchor isn't in the DOM by default and
    the checkpoint has nothing to point at, on master as well. The conversion
    targets the same anchor as the original; verified live by injecting the default
    SequenceNavigation into the slot via env.config.jsx (keepDefault +
    PLUGIN_OPERATIONS.Insert), after which the tour renders and anchors correctly.
  • Manual smoke — done (dev): ✅ new-user modal + Begin tour + AbandonTour on
    skip; ✅ existing-user tour; ✅ launch/replay button; ✅ courseware tour (via slot
    injection — see above); ✅ end-tour PATCH + invalidate GET on completion/dismiss;
    Fix A — dismiss → navigate away/back → modal does not flash back;
    Fix B — window focus mid-tour → no GET, modal does not reopen; ✅ fetch
    gate — /v1/<username> GET fires only on Home + Courseware (not Dates/Progress/
    Discussion; the discussion_tours endpoint is the unrelated UserDiscussionsTours
    feature).

🤖 Generated with Claude Code

@brian-smith-tcril
brian-smith-tcril marked this pull request as ready for review July 31, 2026 19:37
@codecov

codecov Bot commented Jul 31, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.87500% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.34%. Comparing base (1604f36) to head (1fd02ca).

Files with missing lines Patch % Lines
src/product-tours/TourContext.tsx 94.28% 2 Missing ⚠️
src/product-tours/ProductTours.jsx 97.14% 1 Missing ⚠️
Additional details and impacted files
@@                              Coverage Diff                              @@
##           bsmith/react-query-course-recommendations    #1968      +/-   ##
=============================================================================
+ Coverage                                      91.55%   92.34%   +0.79%     
=============================================================================
  Files                                            354      355       +1     
  Lines                                           5824     5852      +28     
  Branches                                        1391     1404      +13     
=============================================================================
+ Hits                                            5332     5404      +72     
+ Misses                                           473      429      -44     
  Partials                                          19       19              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Replace the `tours` Redux slice with React Query for server state and a
React context for the client UI flags (OEP-0067):

- data/apiHooks.ts: useTourData (useQuery) + useEndCourseHomeTour /
  useEndCoursewareTour (useMutation); data/queryKeys.ts keyed by username
- TourContext.tsx: mirrors the former slice's show-flags via useReducer
- ProductTours.jsx / LaunchCourseHomeTourButton.jsx read from the query +
  context instead of Redux; TabPage supplies TourProvider
- remove the tours reducer from store.ts and setupTest.js; delete
  data/{slice,thunks,index}.js

Preserve the original show/hide behavior: invalidateQueries on the end-tour
mutations keeps the cache in sync with the PATCH, and refetchOnWindowFocus:
false prevents a focus refetch from reopening a tour mid-session.

Update tests that render the tour tree to supply TourProvider and, for the
suites that hand-build their tree with RTL render, a QueryClientProvider.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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