| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## master #1993 +/- ##
=======================================
Coverage 92.88% 92.88%
=======================================
Files 364 364
Lines 5943 5943
Branches 1381 1418 +37
=======================================
Hits 5520 5520
Misses 403 403
Partials 20 20 ☔ View full report in Codecov by Harness.
|
Sorry, something went wrong.
There was a problem hiding this comment.
👍🏼
Sorry, something went wrong.
Remove the vestigial `metadataModel` prop from the shared TabPage. Its only live effect was gating the screen-reader-only launch-tour button, which is only ever real on the outline tab — gate that on `activeTabSlug === 'outline'` instead and rename the helper to `renderSrOnlyTourButton`. Drop `metadataModel` from LoadedTabPage and its dead pass-through to StreakCelebrationModal, and from the DatesTab / CoursewareContainer / TabContainer call sites. Move LaunchCourseHomeTourButton's `courseId` from the Redux slice to `useParams`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| Back | FazBrowse Home | New Git URL |
Summary
Simplify the course-home tour button on the shared TabPage by removing the vestigial metadataModel prop. Part of the Redux → React Query migration (#1946); a small, behavior-preserving refactor that lands below the outline-tab conversion (#1991) so outline builds on the cleaned-up TabPage and inherits the courseId fix. Closes #1992.
Investigation showed the screen-reader-only launch-tour button TabPage renders is only ever real on the outline tab: the tour data is fetched only on outline/courseware (long-standing — predates the React Query migration), and course tabs are full page loads, so nothing warms across tabs. metadataModel was a boolean-in-disguise ("is course-home") whose only live effect was gating that button, plus dead threading into LoadedTabPage → StreakCelebrationModal.
What changed
Behavior
No user-facing change. The srOnly button already only materialized on outline (full page loads keep it inert everywhere else), so gating the mount to outline matches reality, and it still renders above the header. The gate behaves identically whether outline is TabContainer-rendered or self-wrapped — both pass activeTabSlug="outline".
Testing
npm run types, npm run lint, and the full npm test suite (106 suites, 902 passing, 3 pre-existing skips) pass. TabPage.test.jsx gains two tests for the new gate (renders the srOnly button on outline, not on other tabs); the call-site test fixtures drop the removed metadataModel prop.
Decisions
Full decision logFindings — what actually renders the course-home tour button
Decision doc for the tour-button cleanup PR — #1993 (sub-issue #1992):
removes the vestigial metadataModel prop and moves the button's courseId off
the Redux slice. Not checked in. The behavior described below is master as
of the investigation; the "Agreed cleanup approach" section is what shipped.
The two render sites of LaunchCourseHomeTourButton
<LaunchCourseHomeTourButton />. CourseTools is rendered only by the
outline tab, so the visible button is outline-only.
<LaunchCourseHomeTourButton srOnly />, gated by
metadataModel === 'courseHomeMeta' (i.e. all course-home tabs, not
courseware) and shouldRenderContent. It's placed above <HeaderSlot>
in the DOM — deliberate reading-order placement inherited from fix: remove launch tour from header #775 (see
History).
What decides whether the button emits any DOM
LaunchCourseHomeTourButton wraps its entire output in {toursEnabled && (…)}
— when toursEnabled is falsy it mounts (its hooks run) but renders an empty
fragment (no DOM).
toursEnabled chain:
useTourData(username, false) — a disabled query. The button never
fetches; it only reads the shared cache entry (tourQueryKeys.user(username)).
useTourData(username, shouldFetchTourData()).
authenticated AND the active tab is outline or courseware AND
(on outline) proctoringPanelStatus === 'loaded'. Its own comment: "Tours
only exist on the Outline and Courseware tabs, so avoid calling the tour
endpoint on any other tab."
on 200; { toursEnabled: false } on 401/403/404 (403 = tour waffle flag off).
The navigation model is the key fact
Course tabs are plain anchors: course-tabs/CourseTabLink.tsx renders
<a href={url}> where url is the backend-supplied courseHomeMeta.tabs[].url
(a full URL — tabs can span MFEs). So switching tabs is a full page load,
not client-side routing. React Router does not intercept a plain <a href>.
Consequences:
starts at its initial null) and a fresh React Query cache.
fetched or set survives the navigation.
ever whatever the current page populated.
The button's data dependencies
page that doesn't run a fetchTab* thunk, this is null.
Net effective behavior (the shape)
Conclusion: the launch-tour button is only ever "real" on the outline tab.
On every other course-home tab the srOnly instance mounts but can never emit DOM
(full page load ⇒ no warmed cache ⇒ toursEnabled undefined). The
metadataModel === 'courseHomeMeta' gate thus mounts a dead button in four
places, and on outline the srOnly button is redundant with the visible
CourseTools one.
History (why it looks like this)
visible button in CourseTools, and the srOnly button. Per the
author's inline review comment on Header.jsx (on the diff, not the PR
conversation thread — easy to miss): "This functions like a 'Skip to main
content' link. Just prompts users to launch the tour if they'd like because
the 'launch tour' button is pretty hidden in the DOM under 'Course Tools'.
Might need to revisit this w/ Jeff Witt to take a second pass at the a11y here,
but this is what we agreed on for now." So the srOnly button was a
skip-link-style a11y aid, added because the visible launch button is
buried under CourseTools (an outline concern), and explicitly flagged as
provisional pending an a11y review that (per the code) never happened. The
tour-data fetch was already gated to outline/courseware here:
userIsAuthenticated && (isCoursewareTab || (isOutlineTab && proctoringPanelStatus === 'loaded')).
Header was being replaced by the shared @edx/frontend-component-header
(no tour logic), so the srOnly button was lifted out of the header into
TabPage, placed above it to preserve the top-of-DOM reading order. This
commit added the metadataModel === 'courseHomeMeta' gate and hardcoded the
button's useModel(...) to 'courseHomeMeta' (before this, metadataModel
actually selected the model — hence the prop's later vestigial drift).
fetch gate: the pre-existing dispatch(fetchTourData) condition was moved
verbatim into shouldFetchTourData() and passed as the RQ query's enabled
flag. It did not change the gating or the behavior.
Corrected conclusion (an earlier draft of this doc/analysis got this wrong):
the srOnly button being effectively outline-only is long-standing, not
something the React Query migration caused — the outline/courseware fetch gate
predates #1968 (identical in the Redux version). And per the #750 rationale the
srOnly button was really an outline a11y aid all along (it substitutes for
the visible button that lives under outline's CourseTools). So the accurate
framing is over-broad mounting, not inversion: metadataModel mounts it on
all five course-home tabs, but it only ever materializes on outline (where it's
redundant with the visible one) and is harmlessly inert elsewhere.
Empirical confirmation (courseware): with the tour armed and the sequence-nav
slot filled, the courseware page shows the courseware tour overlay
(#pgn__checkpoint) but no srOnly launch button — the only
sr-only sr-only-focusable element is the shared header's #main-content
skip-nav link. Consistent with the metadataModel gate excluding courseware.
The courseware tour is a separate mechanism (and also dead-by-default)
Distinct from the launch button. The courseware tour is a guided
ProductTour overlay, not button-launched:
(camelCased from the API's show_courseware_tour).
#courseware-sequence-navigation (product-tours/CoursewareTour.jsx).
Why it doesn't show on a stock install: #courseware-sequence-navigation
lives only in SequenceNavigation.jsx, which is rendered nowhere directly —
it's the would-be content of SequenceNavigationSlot
(org.openedx.frontend.learning.sequence_navigation.v1), a PluginSlot that is
empty by default. No slot fill ⇒ no nav element ⇒ the ProductTour has no
anchor ⇒ nothing renders, regardless of show_courseware_tour. (Confirmed: the
courseware tour appears once the slot is filled — see recipe below.)
Re-arm + view recipe (local dev)
UserTour model (lms/djangoapps/user_tours/models.py): show_courseware_tour
(BooleanField, default True) and course_home_tour_status (choices:
show-new-user-tour / show-existing-user-tour / no-tour).
Re-arm in the LMS Django shell:
Fill the sequence-nav slot so the courseware tour has its anchor (untracked
env.config.jsx, requires a dev-server restart):
Implications for the cleanup
only live effect is gating a button that's only real on outline — plus it's
threaded dead into LoadedTabPage → StreakCelebrationModal (declared
required, never used).
affordance needs to exist at all, and if so where — rather than leaving it
mounted-but-dead on four tabs.
order (the fix: remove launch tour from header #775 intent) — which argues against pushing it down into tab
content.
useSelector(state.courseHome) to useParams — but note it only matters
on outline (the one tab where the button is real and, post-conversion, would
otherwise read a null slice courseId).
Agreed cleanup approach
Its own stack layer, below the outline conversion (so outline builds on the
cleaned TabPage and inherits the courseId fix). Keep the srOnly affordance —
no a11y removal (that would need a deliberate a11y-reviewed change, per the
never-done #750 "revisit w/ Jeff Witt" note); this cleanup only makes the code
say what's already true, with no user-facing behavior change.
(not metadataModel, not a new boolean). activeTabSlug is already a prop;
this directly encodes the headline finding ("only ever on outline") and is
behavior-preserving (the button already only materialized on outline —
full page loads keep it inert elsewhere). Comment it, citing the a11y
rationale inline via the feat: engage product tour #750 review-comment link. Rename renderTourButton →
renderSrOnlyTourButton to make the srOnly variant explicit. Keep it above
HeaderSlot.
LoadedTabPage, the dead threading into StreakCelebrationModal (destructure
(DatesTab, CoursewareContainer, TabContainer's `${slice}Meta`).
useSelector(state.courseHome) → useParams (the de-Redux fix; matters for
the visible outline CourseTools button).
on outline, not on other tabs); drop the removed metadataModel prop from the
call-site fixtures (LoadedTabPage, StreakCelebrationModal, ProductTours,
ProgressTab, OutlineTab).
Works whether outline is still TabContainer-rendered or converted: both pass
activeTabSlug="outline" for the outline route, so the gate behaves identically.
🤖 Generated with Claude Code