Skip to content

refactor: convert course recommendations to React Query - #1967

Open
brian-smith-tcril wants to merge 1 commit into
masterfrom
bsmith/react-query-course-recommendations
Open

refactor: convert course recommendations to React Query#1967
brian-smith-tcril wants to merge 1 commit into
masterfrom
bsmith/react-query-course-recommendations

Conversation

@brian-smith-tcril

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

Copy link
Copy Markdown
Contributor

Summary

Converts the course-exit course recommendations feature from Redux to React Query. This is the pattern-setter PR for the wider Redux → React Query migration tracked in #1946 (per OEP-0067 ADR-0010), following the same path frontend-app-authn and frontend-app-learner-dashboard took.

Behavior is unchanged — recommendations still fetch from discovery and render the same table (or the catalog-suggestion fallback for < 2 results / errors).

What changed

  • React Query infra: add @tanstack/react-query; an app-level QueryClient/QueryClientProvider in src/index.jsx (bare defaults, matching the frontend-base shell's client); and a QueryClientProvider in the shared test render().
  • Colocated data layer: course-exit/data/queryKeys.ts (query-key factory rooted at a new appId constant in src/constants.ts) + course-exit/data/apiHooks.ts (useCourseRecommendations, a useQuery over the existing getCourseRecommendations).
  • Redux removed: delete course-exit/data/slice.js (the recommendations slice) and data/thunks.js; remove the recommendations reducer from src/store.ts and the test store. CourseExit now calls postUnsubscribeFromGoalReminders directly from data/api.js.
  • Consumer: CourseRecommendations branches on React Query flags (isPending/isError/isSuccess) instead of a Redux status string; the sendTrackEvent call moves to a colocated track.js.

Decisions

Full decision log

Scope

  • Pattern-setter = course-exit course recommendations. This PR stands up the
    React Query plumbing and converts the smallest genuinely Redux-backed read
    end-to-end, as the reference the wider Redux → React Query effort (Convert Learning from redux to Context + react-query #1946) copies.
    We deliberately did not use an api-only leaf (preferences-unsubscribe,
    celebration, enrollment-alert) — those use no Redux, so converting them
    wouldn't demonstrate the actual removal. Recommendations is one recommendations
    slice + one thunk + a single consumer (CourseRecommendations.jsx) that reads
    data → useQuery.

Dependency

  • @tanstack/react-query@^5.90.19 as a direct dependency. Major v5 matches
    the reference apps (authn ^5.90.19, LD ^5.90.16) and frontend-base's peer
    requirement (^5.81.2); resolved to 5.101.4. A dependency (not
    peerDependency) because the app bundles its own deps.

React Query client config

  • App-level client is bare: new QueryClient() (no staleTime/retry
    options). Matches the frontend-base shell's own bare client (shell/site.tsx:
    new QueryClient()). We rejected copying the reference apps' values because
    neither is principled and they disagree:

    • LD sets staleTime: 5 * 60_000. Traced to PR chore(i18n): update translations #786: it originally shipped
      staleTime: 60 * 60_000 (1h); reviewer (arbrandes) pushed back ("quite a
      large stale time … default gcTime is 5 min, so it's kind of pointless …
      what's the intended behavior?") and it was reduced to 5 min in a "chore: minor
      improvements" commit — review-nudged, not a real product decision.
    • authn sets retry: false, but for an auth-specific reason (arbrandes: "a
      failed login retried 3 times would be confusing") that doesn't apply to
      reading recommendations.
  • Test client (createTestQueryClient in setupTest.js): retry: false on
    queries + mutations; no gcTime.
    retry: false is universal across the
    references' test setups (authn createWrapper, LD test wrapper, and the shell's
    own test files) — without it a failing query retries 3× with backoff, slowing
    tests and flaking error-path assertions. We dropped gcTime: 0 (LD's test
    wrapper sets it, but authn and the shell tests don't): render() creates a
    fresh client per call, so cache isolation is already guaranteed. The
    bare-app-client / configured-test-client split mirrors the shell (bare
    site.tsx client, configured test files).

  • QueryClientProvider placement. Nested just inside AppProvider
    (AppProvider > QueryClientProvider > …) in both src/index.jsx and the shared
    test render(). Matches LD's ordering; AppProvider stays the outermost app
    wrapper.

Query keys

  • Colocated per-feature, rooted at a shared appId constant. Keys live in
    each feature's data/queryKeys.ts (here course-exit/data/queryKeys.ts), rooted
    at appId from src/constants.ts ([appId, '<feature>', …]). We chose this
    over LD's fully-centralized src/data/ data layer:
    • Learning already organizes by feature (≈a dozen existing feature data/
      dirs), so colocation matches the codebase and keeps each conversion PR local
      and low-conflict; a central keys file would be a hot shared file and a large
      upfront structural move.
    • Rooting at a shared appId constant (authn's discipline —
      import { appId } from '../../constants') gives collision-safety and a
      consistent namespace without the centralization churn.
    • Keys can be lifted into src/data/ later if cross-feature invalidation
      actually becomes common (it rarely does).
    • Added export const appId = 'learning'; to src/constants.ts (value matches
      the legacy APP_ID env var).

Consumer conversion (CourseRecommendations.jsx)

  • Branch on React Query's flags directly (isPending/isError/isSuccess),
    not a derived legacy status string. Matches the reference repos, which use RQ
    flags in components (e.g. LD Dashboard: const { data, isPending } = …;
    MasqueradeBar: !isError && !isPending). An earlier draft derived a
    recommendationsStatus (LOADING/LOADED/FAILED) to keep the old branch
    conditions; dropped as un-idiomatic. Behavior is identical
    (LOADING→isPending, FAILED→isError, LOADED→isSuccess).
  • Tracking extracted to a colocated track.js (feature root), following
    authn's src/recommendations/track.js pattern: trackRecommendationsViewed({ courseKey, isError, length }) owns the sendTrackEvent call and the
    FAILED/LOADED status-string mapping — the one place the legacy status strings
    are still needed, purely for analytics continuity (the event reports the same
    recommendations_status values as before). Consequence:
    CourseRecommendations.jsx no longer imports sendTrackEvent or
    @src/constants. Chose this over LD's heavier central src/tracking/ +
    useCourseTrackingEvent machinery — overkill for one event.
  • courseId still read from the courseware Redux slice via useSelector
    that slice isn't part of this conversion, and reading a not-yet-converted slice
    is expected during the incremental effort. Dropped useDispatch, the fetch
    useEffect, and useModel('coursewareMeta').recommendations (the query owns the
    data now).
  • Used recommendations.length directly (dropped the recommendationsLength
    local and the old recommendations ? … : 0 guard): the query's data = []
    default guarantees an array, so the guard was dead code.

Data layer

  • Deleted data/slice.js (the recommendations slice) and removed its reducer
    from store.ts and setupTest.js's initializeTestStore.
  • Deleted data/thunks.js entirely. After removing the recommendations thunk,
    its only remaining export was unsubscribeFromGoalReminders — a redundant
    wrapper around postUnsubscribeFromGoalReminders (already in api.js), and
    CourseExit.jsx only passed courseId. So CourseExit.jsx now calls
    postUnsubscribeFromGoalReminders directly from data/api.js. The unsubscribe
    test asserts on the POST, so behavior is unchanged.
  • Reused data/api.js getCourseRecommendations unchanged as the queryFn
    (it fetches recommendations + enrollments and filters). Pact/api tests keep
    testing it.

Out of scope

Test plan

  • npm run lint, npm run types, npm run build — clean.
  • npm test — full suite green (100 suites / 868 passed / 3 skipped), including CourseExit.test.jsx's recommendations coverage: success→table (with already-enrolled / same-course filtering), and < 2/error→catalog-suggestion fallback.
  • Manual, against a live Tutor + discovery backend: on the course celebration page, the recommendations table renders from a real discovery course_recommendations response through the converted useQuery; also verified the fallback path when discovery returns < 2 / errors.

Refs #1946

🤖 Generated with Claude Code

@brian-smith-tcril brian-smith-tcril changed the title bsmith/react query course recommendations refactor: convert course recommendations to React Query Jul 31, 2026
@brian-smith-tcril
brian-smith-tcril marked this pull request as ready for review July 31, 2026 19:37
Replace the Redux `recommendations` slice + thunk with a
`useCourseRecommendations` React Query hook, as the pattern-setter for the
wider Redux -> React Query migration (#1946).

- Add the app-level QueryClient/QueryClientProvider (bare, matching the
  frontend-base shell) and a QueryClientProvider wrapper in the test render.
- Add a colocated data layer: data/queryKeys.ts (rooted at a new `appId`
  constant) + data/apiHooks.ts (useQuery over the existing getCourseRecommendations).
- Delete data/slice.js and data/thunks.js; CourseExit now calls
  postUnsubscribeFromGoalReminders directly from data/api.js.
- Remove the `recommendations` reducer from the store and the test store.
- CourseRecommendations branches on React Query flags (isPending/isError/
  isSuccess) instead of a status string; the tracking event moves to a
  colocated track.js. Behavior is unchanged.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@brian-smith-tcril
brian-smith-tcril force-pushed the bsmith/react-query-course-recommendations branch from 10eb2bd to 1604f36 Compare July 31, 2026 19:41
@codecov

codecov Bot commented Jul 31, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.55%. Comparing base (c694ac6) to head (1604f36).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1967      +/-   ##
==========================================
+ Coverage   91.48%   91.55%   +0.06%     
==========================================
  Files         353      354       +1     
  Lines        5838     5824      -14     
  Branches     1356     1391      +35     
==========================================
- Hits         5341     5332       -9     
+ Misses        478      473       -5     
  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.

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