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) thank you Sol!
• 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.40845% with 276 lines in your changes missing coverage. Please review.
✅ Project coverage is 42.24%. Comparing base (f656c73) to head (8cea0ce).
⚠️ Report is 2 commits behind head on main.

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/state/actions/index.tsx	16.66%	15 Missing ⚠️
src/app/components/navigation/IsaacApp.tsx	63.63%	12 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 ⚠️
.../app/components/elements/sidebar/EventsSidebar.tsx	14.28%	6 Missing ⚠️
...rc/app/components/pages/RegistrationSetDetails.tsx	25.00%	6 Missing ⚠️
... and 61 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1898      +/-   ##
==========================================
- Coverage   42.35%   42.24%   -0.12%     
==========================================
  Files         575      575              
  Lines       24396    24474      +78     
  Branches     8069     7198     -871     
==========================================
+ Hits        10334    10339       +5     
- Misses      13398    14089     +691     
+ Partials      664       46     -618     

☔ 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.

[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.

[sjd210]

Very nearly there! Just some small comments on your most recent changes now.

[sjd210]

This is stopping ALL users from accessing questions!

Suggested change

	<Route path="/questions/:questionId" element={<RequireAuth auth={isTeacherPending} element={<Question />} />} />
	<Route path="/questions/:questionId" element={<RequireAuth auth={(user) => !isTeacherPending(user)} element={<Question />} />} />

Although this does raise an interesting point that RequireAuth already has its own isTeacherPending check that kicks in before this auth ever does. I don't think there's actually much benefit in having an auth-less RequireAuth, so just food for thought

[jacbn]

Ah, missed a few of these negations. I was changing both what the function did and negating its name, got a little confusing 😔. Thanks for spotting.

Your second point is interesting. The overall effect is essentially the same as there being an implicit <RequireAuth auth={(user) => !isTeacherPending(user)} element={...} /> wrapped around every other <RequireAuth /> currently, so I suppose one way we could restructure the routes to get around this is to use the nested-auth approach mentioned at the bottom of the original comment on this PR to make this implicit check explicit, then remove the !isTeacherPending check from inside the <RequireAuth /> component.

It's quite a big change given we need to move all the requiring-auth routes together in IsaacApp, so I'd probably want to do it separately to this PR (if we even want to). For now, I think I prefer your suggestion here, since even though this logic is never used, it makes it obvious why we need this route to require authentication, which is something we don't get with an auth-element-less <RequireAuth />.

[sjd210]

Hrm, this is tricky. There are a couple of alternative ways of doing this that I could think of but I'm not particularly fond of any of them.

I too am surprised that the navigateComponentless function seems to be just working (and I think we should keep an eye on it, in case something does start going weird) but I'm happy with that! I would probably prefer moving all of the RootLayout + roots + navigateComponentless to a new file in the navigate folder, but practically speaking that doesn't make much difference.

[sjd210]

Ah fair enough. I will admit I hadn't actually checked whether what I had written worked.

I'm much happier with this new approach not having the questionable type guard and inconsistent language, so I certainly won't complain (except a little about the associated comment, see below).