| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
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>
Codecov Report✅ All modified and coverable lines are covered by tests. @@ 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.
|
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM!
Sorry, something went wrong.
Convert CoursewareContainer from a class + connect component to a functional
component using hooks. Structural only — no data-layer or behavior change: it
dispatches the same thunks and reads the same store. This unblocks the later
courseware React Query peels, which need a function component to call query
hooks a class can't.
- class -> function; connect -> useSelector/useDispatch
- inline the route hooks (useParams/useNavigate/useLocation) and delete the
withParamsAndNavigation HOC (utils.jsx), its only consumer
- keep the reselect memoize fetch/save guards (held in refs, reading a
per-render latest ref) inside one no-dependency effect, matching the old
componentDidMount + componentDidUpdate exactly
- drop the dead previousSequence selector/prop (unused; keep the empty
previousSequenceHandler that Course still requires)
- guard useIFrameBehavior's activeSequence.unitIds read: removing connect lets
the global getSequenceId briefly lead the rendered sequenceId prop, so the
sequence model can transiently be the outline stub (no unitIds)
- drop dead fetchCourseRecommendations{Request,Success,Failure} exports
(orphaned from the recommendations RQ conversion in #1967)
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Convert CoursewareContainer from a class + connect component to a functional
component using hooks. Structural only — no data-layer or behavior change: it
dispatches the same thunks and reads the same store. This unblocks the later
courseware React Query peels, which need a function component to call query
hooks a class can't.
- class -> function; connect -> useSelector/useDispatch
- inline the route hooks (useParams/useNavigate/useLocation) and delete the
withParamsAndNavigation HOC (utils.jsx), its only consumer
- keep the reselect memoize fetch/save guards (held in refs, reading a
per-render latest ref) inside one no-dependency effect, matching the old
componentDidMount + componentDidUpdate exactly
- drop the dead previousSequence selector/prop (unused; keep the empty
previousSequenceHandler that Course still requires)
- guard useIFrameBehavior's activeSequence.unitIds read: removing connect lets
the global getSequenceId briefly lead the rendered sequenceId prop, so the
sequence model can transiently be the outline stub (no unitIds)
- drop dead fetchCourseRecommendations{Request,Success,Failure} exports
(orphaned from the recommendations RQ conversion in #1967)
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Convert CoursewareContainer from a class + connect component to a functional
component using hooks. Structural only — no data-layer or behavior change: it
dispatches the same thunks and reads the same store. This unblocks the later
courseware React Query peels, which need a function component to call query
hooks a class can't.
- class -> function; connect -> useSelector/useDispatch
- inline the route hooks (useParams/useNavigate/useLocation) and delete the
withParamsAndNavigation HOC (utils.jsx), its only consumer
- keep the reselect memoize fetch/save guards (held in refs, reading a
per-render latest ref) inside one no-dependency effect, matching the old
componentDidMount + componentDidUpdate exactly
- drop the dead previousSequence selector/prop (unused; keep the empty
previousSequenceHandler that Course still requires)
- guard useIFrameBehavior's activeSequence.unitIds read: removing connect lets
the global getSequenceId briefly lead the rendered sequenceId prop, so the
sequence model can transiently be the outline stub (no unitIds)
- drop dead fetchCourseRecommendations{Request,Success,Failure} exports
(orphaned from the recommendations RQ conversion in #1967)
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Convert CoursewareContainer from a class + connect component to a functional
component using hooks. Structural only — no data-layer or behavior change: it
dispatches the same thunks and reads the same store. This unblocks the later
courseware React Query peels, which need a function component to call query
hooks a class can't.
- class -> function; connect -> useSelector/useDispatch
- inline the route hooks (useParams/useNavigate/useLocation) and delete the
withParamsAndNavigation HOC (utils.jsx), its only consumer
- keep the reselect memoize fetch/save guards (held in refs, reading a
per-render latest ref) inside one no-dependency effect, matching the old
componentDidMount + componentDidUpdate exactly
- drop the dead previousSequence selector/prop (unused; keep the empty
previousSequenceHandler that Course still requires)
- guard useIFrameBehavior's activeSequence.unitIds read: removing connect lets
the global getSequenceId briefly lead the rendered sequenceId prop, so the
sequence model can transiently be the outline stub (no unitIds)
- drop dead fetchCourseRecommendations{Request,Success,Failure} exports
(orphaned from the recommendations RQ conversion in #1967)
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
| Back | FazBrowse Home | New Git URL |
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
Decisions
Full decision logScope
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
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:
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.
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
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:
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.
import { appId } from '../../constants') gives collision-safety and a
consistent namespace without the centralization churn.
actually becomes common (it rarely does).
the legacy APP_ID env var).
Consumer conversion (CourseRecommendations.jsx)
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).
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.
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).
local and the old recommendations ? … : 0 guard): the query's data = []
default guarantees an array, so the guard was dead code.
Data layer
from store.ts and setupTest.js's initializeTestStore.
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.
(it fetches recommendations + enrollments and filters). Pact/api tests keep
testing it.
Out of scope
useMutation pattern and the rest of learning's Redux surface are converted
separately under the wider effort tracked in Convert Learning from redux to Context + react-query #1946.
Test plan
Closes #1972
Refs #1946
🤖 Generated with Claude Code