Use useGoBack for course back navigation instead of hardcoded HOME#14403
Use useGoBack for course back navigation instead of hardcoded HOME#14403nucleogenesis wants to merge 3 commits intolearningequality:release-v0.19.xfrom
Conversation
CourseWelcomePage hardcoded its back arrow to always navigate to HOME, ignoring where the user came from. Fix by: - Adding CourseRootPage as a route-level wrapper that calls usePreviousRoute(), providing previous route tracking for all course child routes - Nesting course routes as children of CourseRootPage - Replacing the hardcoded homePageLink in CourseWelcomePage with useGoBack(), falling back to HOME when no previous route exists This follows the same pattern used in Facility (UsersRootPage) and Coach (QuizResourceSelection, LessonResourceSelection) plugins. Fixes learningequality#14222 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The Kolibri router's handler system only extracts handlers from top-level routes, not children. Since course routes are now nested under CourseRootPage, the noClassesGuard handler was not firing. Switch to Vue Router's native beforeEnter guard on the parent route, which fires for all child route navigations. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
rtibblesbot
left a comment
There was a problem hiding this comment.
Clean bugfix that correctly adopts the established usePreviousRoute/useGoBack pattern for course back navigation.
CI: Python tests passing. Frontend tests, linting, and WHL build still pending — verify before merge.
- suggestion (1): Bare
/coursepath is now routable — see inline comment onclassesRoutes.js:90 - praise (2): See inline comments
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Reviewed the pull request diff checking for:
- Correctness: bugs, edge cases, undocumented behavior, resource leaks, hardcoded values
- Design: unnecessary complexity, naming, readability, comment accuracy, redundant state
- Architecture: duplicated concerns, minimal interfaces, composition over inheritance
- Testing: behavior-based assertions, mocks only at hard boundaries, accurate coverage
- Completeness: missing dependencies, unupdated usages, i18n, accessibility, security
- Principles: DRY (same reason to change), SRP, Rule of Three (no premature abstraction)
- Checked CI status and linked issue acceptance criteria
- For UI changes: inspected screenshots for layout, visual completeness, and consistency
| name: PageNames.COURSE_WELCOME, | ||
| path: '/course/:courseSessionId([a-f0-9]{32})/welcome', | ||
| component: CourseWelcomePage, | ||
| path: '/course', |
There was a problem hiding this comment.
suggestion: The parent route path: '/course' is now a routable URL, but it has no name, no redirect, and no default child. If a user navigates to /course directly, they'll see CourseRootPage rendering an empty <router-view /> — a blank page.
No UI link currently points to bare /course, so this is unlikely to be hit in practice. But a defensive redirect (e.g., to HOME) would prevent the blank page if someone bookmarks or manually enters the URL. Low priority — just something to be aware of.
| export default { | ||
| name: 'CourseRootPage', | ||
| setup() { |
There was a problem hiding this comment.
praise: Minimal wrapper following the exact same pattern as UsersRootPage in Facility — just usePreviousRoute() + <router-view />. Good restraint keeping it thin.
| :appBarBgColor="$themeTokens.surface" | ||
| :appBarHoverBgColor="$themePalette.grey.v_100" | ||
| :appearanceOverrides="{ backgroundColor: $themeTokens.surface }" | ||
| @navIconClick="goBack" |
There was a problem hiding this comment.
praise: Good switch from :route to @navIconClick. ImmersiveToolbar renders a static <router-link> when route is provided, but useGoBack needs runtime logic to choose between router.back() and the fallback — so the event handler is the correct mechanism.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
rtibblesbot
left a comment
There was a problem hiding this comment.
Clean follow-up addressing the prior suggestion.
Delta from prior review
- RESOLVED: bare
/courseblank page —redirect: '/'added (d5be2cb) - New:
beforeEnterconsolidation moves thenoClassesGuardto the parent route, eliminating six identicalhandlerwrappers. Good simplification.
CI: build assets still in progress — verify before merge.
No new findings.
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Compared the current PR state against findings from a prior review:
- Retrieved prior bot reviews deterministically via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Reviewed the pull request diff checking for correctness, design, architecture, testing, completeness, and adherence to DRY/SRP principles
- Checked CI status and linked issue acceptance criteria
| return; | ||
| } | ||
| next(); | ||
| }, |
There was a problem hiding this comment.
praise: Clean consolidation — six identical handler wrappers replaced with a single beforeEnter on the parent. The guard only needs to check class membership on entry to the /course subtree, not on every sibling navigation, so this is both simpler and semantically correct.
Build Artifacts
Smoke test screenshot |
Summary
CourseWelcomePage hardcoded its back arrow to always navigate to the Home page,
regardless of where the user came from. This fix adopts the existing
useGoBack/usePreviousRoutepattern (used in Facility and Coach plugins)for course navigation:
CourseRootPagewrapper component callsusePreviousRoute()and renders<router-view />CourseRootPage, enabling previous route trackingCourseWelcomePageusesuseGoBack({ fallbackRoute: HOME })instead of a hardcoded HOME linkThis follows the same pattern as
UsersRootPage(Facility),QuizResourceSelectionandLessonResourceSelection(Coach).References
Fixes #14222
Review guidance
QA guidance
Setup prerequisites:
Test path 1: From Classes page
Test path 2: From Home page
Test path 3: Regression tests on 'no classes guard'
Note that the other case I tested was for a logged-in user trying to go to a class URL for a class they're not in and it resulted in a 404 Error which is right because to the user not in the class, the class doesn't exist.
Code Reviewer notes (from Claude)
CourseRootPageis a thin wrapper (~15 lines) following an established pattern — compare withUsersRootPagein Facility orQuizResourceSelectionin CoachCourseRootPage, with thenoClassesGuardhandler consolidated on the parent route. URL paths are unchanged.useContentLinkquery-param approach (used by TopicsPage for deepcontent browsing), but
useGoBackis a better fit here — CourseWelcomePage is simple"go back one level" navigation, not deep content browsing
useGoBackpattern relies ononBeforeRouteUpdatefiring onCourseRootPagewhenCourseUnitViewdoesrouter.replacetoCourseWelcomePage(both are child routes).This sets
previousRoute, enablingrouter.back()to return to the correct origin page.When no previous route exists (direct link/refresh), it falls back to HOME.
AI usage
I used Claude Code to investigate the root cause, trace the navigation chain, and explore
two approaches: query-param threading via
useContentLinkand theuseGoBack/usePreviousRoutepattern. After testing both, we choseuseGoBackas the better fitfor this use case. I reviewed the generated code and verified it follows the same pattern
used elsewhere in the codebase.