Upgrade to React Router v7

[jacbn]

(n.b. this branch is over the top of the Vite one. we should rebase if we want to merge this in first)

Upgrades React Router to v7.11.0. The main changes come from v5 => v6; you can read migration guides for v5 => v6 and v6 => v7 in detail on the RR docs. For us, the main changes are:

useHistory

The docs are far more complete than what I have written. Read if needed.

useHistory has been replaced by useNavigate. To me, it seems like the aim is to better distinguish history into getting information about the current URL (for which useLocation can still be used) and push/replace things on the history stack (for which useNavigate is now used).

/* old, RR5 */
const history = useHistory();
history.push("/groups");
history.replace("/login");
console.log(history.location);

/* new, RR7 */
const navigate = useNavigate();
const location = useLocation();
navigate("/groups");
navigate("/login", {replace: true});
console.log(location);

Also, some other things navigate can do:

navigate(-1); // go back
navigate({pathname, search, key, hash}, {state, replace}); // use search objects directly

Note that state is in the options param in the last one.

Top-level history

There is no longer an accessible top-level, out-of-component history object that is shared by the router. window.history still exists but is the HTML5 API as opposed to the old History object, so has different syntax. From my testing, it seems like state pushed to the HTML5 one is not detectable by the router's useLocation hook, so we should always prefer useNavigate over any history.pushState, and it is required to do so when state is involved. There are some situations where replacement is not easy owing to useNavigate being a hook, so not all have been replaced yet – but this is something to work towards.

Routes and TrackedRoutes

RR6+ allows only <Route/> objects inside the <Routes/> (previously <Switch/>) component. This is strict – not even components that produce a <Route/> are allowed. As such, <TrackedRoute /> has been entirely removed.

<TrackedRoute/> as a component did 3 things on top of a regular <Route/>, all of which have necessarily moved:

1. Track the route, upload data to Plausible
2. Manage user authentication
3. Provide a fresh FigureNumberingContext to child components.

1. Tracking

The tracking inside each TrackedRoute essentially boiled down to this:

useEffect(() => {
    trackPageview(location);
}, [location])

All this does is track a page view each time the location changes. We already have an OnPageLoad component which does things like manage scrolling to the top or resetting focus when the location changes, so I have moved the logic for this there. The downside(?) of this approach is that we can't 'pick' which pages get tracked and which don't – everything does, always. I'm not certain whether there was a reason we weren't doing this for all routes before, but this is a noticeable difference.

2. Authentication

The authentication logic has been moved into a new component <RequireAuth />; this needs to exist inside the replacement <Route />'s element prop, where the auth prop is the same as the old ifUser prop. e.g:

/* old, RR5 */
<TrackedRoute path="/account" ifUser={isLoggedIn} component={MyAccount} />

becomes

/* new, RR7 */
<Route path="/account" element={
    <RequireAuth auth={isLoggedIn} element={<MyAccount />} />
}/>

This seems overcomplicated (why are we introducing a new component for this?), but bear with me. RR is moving towards a "element-first" approach (see this for their explanation), i.e. they prefer element={<MyAccount />} over component={MyAccount}. The main advantage is being able to pass props directly, rather than requiring the old-style componentProps functionality. Unfortunately, this throws a bit of a spanner in our type system. Before, since user was validated to a LoggedInUser inside the auth logic, we could pass a validated user to a component there, meaning the user type was known to never be undefined / null / not logged in. With the new approach, we need the user at the top level, since we want to pass it in when we create the element:

<Route path="/groups" element={
    <RequireAuth auth={isTutorOrAbove} element={<Groups user={/* ??? */} />} />
    /* Any top-level user we can pass in here would not be typed as if it were authenticated! */
}/>

This is the advantage of having a separate authentication component: <RequireAuth /> allows elements to be defined as a function, producing the result given the authenticated user:

<Route path="/groups" element={
    <RequireAuth auth={isTutorOrAbove} element={(authUser) => <Groups user={authUser} />} />
}/>

<RequireAuth /> then runs the authentication logic and, if it passes, renders this component with the correctly-typed user. If it does not pass, the usual redirects are used instead.

3. FigureNumberingContext

The last feature that TrackedRoute provided was wrapping each page in a fresh figure numbering context. Since we want this on every page and putting it inside the Routes where they were before requires pasting it into each component separately, I have moved the logic above the routes, wrapping the entire <Routes/> component in one context and resetting it on location change. It's worth testing that this still works in all cases.

Redirects

In short, <Redirect to="..." />s are replaced with <Navigate to="..."/>. A minor difference is that Redirects used to work as top-level <Route /> components, whereas now since every top-level component in a <Routes/> block must be a <Route/>, <Navigate />s must be wrapped inside a <Route/>. The old from prop inside the Redirect now exists as the Route's path prop.

/* old, RR5 */
<Redirect from="/pages/glossary" to="/glossary" /> 

/* new, RR7 */
<Route path="/pages/glossary" element={
    <Navigate to="/glossary" replace /> 
}/>

Note the replace – the default behaviour in RR5 was to replace a history entry. In RR7, the default behaviour is to push a new one, so if we want to maintain the old behaviour we need the replace prop.

isRole function types

These functions haven't changed, but I have replaced the => boolean types with user is LoggedInUser & {readonly role: "ROLE"} type guards. This enables the correct typing of authUser when used as the auth prop in <RequireAuth />. The & {role...} type is simply to prevent the following from inferring user as type never:

if (isLoggedIn(user) && !isStudent(user)) {...}

---

To consider:

• It would be possible to group <Routes /> by authentication requirement using an <Outlet /> as the return of <RequireAuth />, which would limit the number of <RequireAuth />s to as many roles as there are across the entire app – see this SO answer. Thoughts?

---

To fix:

• logging in requires a reload to show content? (ada)
• blocking on page leave - see TODOs
  (these are small! reviewing what is here is possible in the meantime)

[codecov]

Codecov Report

❌ Patch coverage is 51.52672% with 254 lines in your changes missing coverage. Please review.
✅ Project coverage is 42.65%. Comparing base (c3e7708) to head (ccd8742).

Files with missing lines	Patch %	Lines
src/app/components/content/IsaacContent.tsx	29.16%	17 Missing ⚠️
src/app/components/pages/GameboardBuilder.tsx	0.00%	16 Missing ⚠️
src/app/components/site/phy/RoutesPhy.tsx	40.00%	15 Missing ⚠️
src/app/components/navigation/IsaacApp.tsx	65.51%	10 Missing ⚠️
src/app/components/site/cs/RoutesCS.tsx	47.05%	9 Missing ⚠️
...ents/elements/sidebar/ContentControlledSidebar.tsx	22.22%	7 Missing ⚠️
src/app/components/pages/Support.tsx	36.36%	7 Missing ⚠️
src/app/state/actions/index.tsx	22.22%	7 Missing ⚠️
.../app/components/elements/sidebar/EventsSidebar.tsx	14.28%	6 Missing ⚠️
...rc/app/components/pages/RegistrationSetDetails.tsx	25.00%	6 Missing ⚠️
... and 57 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1898      +/-   ##
==========================================
- Coverage   42.93%   42.65%   -0.28%     
==========================================
  Files         572      572              
  Lines       23971    24039      +68     
  Branches     7877     7052     -825     
==========================================
- Hits        10291    10255      -36     
- Misses      13041    13738     +697     
+ Partials      639       46     -593     

☔ View full report in Codecov by Sentry.
📢 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.

[sjd210]

I spent a while looking at this branch, but annoyingly half the issues I found were caused by a merge conflict that was my fault rather than anything to do with your changes 🙃.

Anyway, the branch is mostly pretty good. I quite like the changes that React Router makes from v5 -> v6 in general and TrackedRoute was already quite bloated by including things other than tracking so I like it being separated out anyway.

The most significant thing that's actually not working is the various history.pushStates around the code. The rest is just some of my style preferences, or errors in tests from your last-minute page-blocking change. Nothing too much :)

[sjd210]

This annoys me, and it already did before the type guard.

• Firstly, if we're only using it as a "fully logged in" check then we should actually enforce that here. I don't see any reason we'd want to specifically check for a user who is either fully logged in or not at all (and if so we should be more explicit than just using this function) - and changing it means then this new type guard would match.
• Secondly, "partially logged in" is a term used in the backend to refer to users needing verification OR in the middle of the MFA process. This isn't currently consistent with the MFA part.
• Thirdly, I just find it a little odd style-wise to have a negative asserting function among otherwise only positive ones.

I'd much prefer something similar like this, which addresses all of those points:

export function isFullyLoggedIn(user?: {readonly role?: UserRole, readonly loggedIn?: boolean, readonly teacherAccountPending?: boolean, readonly EMAIL_VERIFICATION_REQUIRED?: boolean} | null): user is LoggedInUser {
   return isLoggedIn(user) && !(isAda && (isTeacherAccountPending(user) || user?.EMAIL_VERIFICATION_REQUIRED));
}

(+ this allows us to slightly simplify some expressions in the rest of the code by removing the extra logged in check)

[jacbn]

The type doesn't work in the one you've provided as the user roles are inconsistent. I've done some cleanup to make it so we can use this but it's not as easy as I was hoping 😣

[sjd210]

This is what's causing the page content to be unable to load after logging in. history.pushState is able to set the browser's url state to the target url but doesn't actually do anything to load the associated page (at least not without manually reloading, or something like fireEvent(window, new PopStateEvent('popstate')); as we have in test files).

This is causing issues for most if not all of the history.pushStates that you add in this PR - superseded questions are quite an easy one to test it on. I think that we should just be using a void navigate in each of these scenarios, even if that means tunneling the navigate through from a component using the hook as we do for handleProviderCallback above. I'd also just prefer that for consistency anyway.

[jacbn]

While I agree with the latter point in principle, and while a couple of the remaining instances of pushState are probably doable with some horrible drilling, I'm not sure it's actually possible to get a valid navigate instance inside the RTK slices, which use navigation here and here. The types and fn arguments etc. all appear to come from the RTK library itself. So I'm fairly sure we need some other way of doing it, regrettably.

For your first point, yep, that makes sense. fireEvent is a RTL thing so we can't use it outside of tests, but I suppose a window.refresh() or something would be equivalent. So having one of those after each is (a horrible) option #1.

As an alternative, I've found that the router object returned by createBrowserRouter actually does provide a .navigate function directly which we can use. Unfortunately owing to its position in the import hierarchy, we can't just import { router } from ... without creating circular dependencies. I've done something a little horrible and put a navigateComponentless function on window – initially as a proof of concept and now because I can't think of anything better – when the router gets created. This... seems to work! I really don't like where it is and would much prefer importing it from somewhere but I can't think of anywhere more suitable. Any ideas?

[jacbn]

There's a single test still failing on this, but I have to run now. Feel free to pick up if anyone can, I'll take a look on Monday if not.