Skip to content

Commit

Permalink
Improve frontend performance by avoiding useless renders (#1321)
Browse files Browse the repository at this point in the history
The original goal was to speed up the search page (since it was rendered
multiple times, uselessly), but these changes benefit all of Tobira.
This generally cuts down on duplicate renders and useless work that is
done. You can see this effect by adding `console.log` statements to
`ActiveRoute`, a route of your choice like `RealmPage` and some other
places. Do make sure to remove `<StrictMode>` for testing, as that
doubles all renders again, making it harder to see. It's also helpful to
add `tokio::time::sleep(std::time::Duration::from_millis(1500)).await;`
to `handle_api` to easily see loading states.

This is a bit infuriating as it's still not perfect. But I cannot find a
way to fix the last bit and searching for in-depth information on this
stuff is super hard. I only find surface-level resources on this. Even
blog posts which title contains "in-depth" or "deep dive" are laughably
shallow. The react docs are good in principle but leave out some behind
the scene information that would aid with debugging. The React dev tools
are also surprisingly unhelpful.

Here is some information on what is rendered when:

#### Initial load

```
----- send GQL
Render <Router>
Render <ActiveRoute />  @1
RealmRoute::render /lectures/d-chab/2015/autumn/529-0010-00L
Render <RootLoader>
----- network arrived
Render <ActiveRoute />  @1
RealmRoute::render /lectures/d-chab/2015/autumn/529-0010-00L
Render <RootLoader>
Render <RealmPage /> @9 Chemistry
```

That's actually perfect as far as I can tell. First the request is sent,
then the router routes and tries to render the active route, which leads
to `<RootLoader>` suspending (then showing <`InitialLoading />`). Once
the response arrives, the child of `<Suspense>` (`<ActiveRoute>`) is
rerendered, and this time `RootLoader` does not suspend, so the
RealmPage is rendered (only once!).

#### Clicking on a link

```
----- send GQL
Render <Router> {isPending: true, ar: '@1', sar: '@2', nav: '@3', lis: '@4', …}
Render <Router> {isPending: false, ar: '@10', sar: '@2', nav: '@3', lis: '@4', …}
Render <ActiveRoute />  @10
RealmRoute::render /lectures/d-chab/2015/autumn
Render <RootLoader>
Render <Router> {isPending: false, ar: '@10', sar: '@2', nav: '@3', lis: '@4', …}
Render <ActiveRoute />  @10
RealmRoute::render /lectures/d-chab/2015/autumn
Render <RootLoader>

----- network arrived
Render <RootLoader>
Render <RootLoader> after relay 1 @6 @7 @8
Render <Router> {isPending: false, ar: '@10', sar: '@2', nav: '@3', lis: '@4', …}
Render <ActiveRoute />  @10
RealmRoute::render /lectures/d-chab/2015/autumn
Render <RootLoader>
Render <RootLoader> after relay 0 @11 @12 @13
Render <RealmPage /> @14 Autumn
```

Good: the GQL request is sent first thing. Then the router is rerendered
because `isPending` switches to `true`, which causes the loading
indicator to do its thing. But:

Before the response arrives, we already have one duplicate rendering.
Why is the router rendered twice with exact same props and all? This
goes down to the `RootLoader` twice, which suspends both times I
suspect. I actually know how to make it go away: Removing
`<LoadingIndicator>` completely gets rid of one duplication (the last
four lines before network arrives). But all the ways I tried to add
loading indicator back again resulted in a duplicate render again. But
oh well, at least the actual `<RealmPage>` is not rendered, so it should
all be fairly cheap.

Secondly, after the response arrives, the `RootLoader` is rendered for
an unknown reason. If it weren't for the last commit, that would
rerender the `RealmPage` with the previous data! No idea why. And then
everything is freshly rendered starting with `Router` down to
`RealmPage`.

So... as far as I can see, the actual page (`RealmPage` in this case) is
not rendered uselessly. Which is already a success. But still, weird
stuff is happening and as I said, it's infuriating.
  • Loading branch information
owi92 authored Jan 24, 2025
2 parents 0de3ca3 + 7296516 commit e5af633
Show file tree
Hide file tree
Showing 6 changed files with 64 additions and 36 deletions.
7 changes: 5 additions & 2 deletions frontend/src/App.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, { ReactNode, StrictMode } from "react";
import React, { ReactNode, StrictMode, Suspense } from "react";
import { RelayEnvironmentProvider } from "react-relay/hooks";
import { CacheProvider } from "@emotion/react";
import createEmotionCache from "@emotion/cache";
Expand All @@ -19,6 +19,7 @@ import {
} from "@opencast/appkit";
import { COLORS } from "./color";
import { InitialConsent } from "./ui/InitialConsent";
import { InitialLoading } from "./layout/Root";


type Props = {
Expand All @@ -39,7 +40,9 @@ export const App: React.FC<Props> = ({ initialRoute, consentGiven }) => (
<MenuProvider>
<LoadingIndicator />
<InitialConsent {...{ consentGiven }} />
<ActiveRoute />
<Suspense fallback={<InitialLoading />}>
<ActiveRoute />
</Suspense>
</MenuProvider>
</GraphQLErrorBoundary>
</Router>
Expand Down
24 changes: 13 additions & 11 deletions frontend/src/layout/Root.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, { ReactNode, Suspense, useEffect, useRef } from "react";
import React, { ReactNode, useEffect, useMemo, useRef } from "react";
import { keyframes } from "@emotion/react";
import { useTranslation } from "react-i18next";
import { screenWidthAtMost } from "@opencast/appkit";
Expand Down Expand Up @@ -130,13 +130,7 @@ type RootLoaderProps<Q extends QueryWithUserData> = {
};

/** Entry point for almost all routes: loads the GraphQL query and renders the main page layout */
export const RootLoader = <Q extends QueryWithUserData>(props: RootLoaderProps<Q>) => (
<Suspense fallback={<InitialLoading />}>
<RootLoaderImpl {...props} />
</Suspense>
);

export const RootLoaderImpl = <Q extends QueryWithUserData>({
export const RootLoader = <Q extends QueryWithUserData>({
query,
queryRef,
nav,
Expand All @@ -159,11 +153,19 @@ export const RootLoaderImpl = <Q extends QueryWithUserData>({
return undefined;
}));

// Unfortunately, `<ActiveRoute />` and `<RootLoader />` are still rendered
// more than they need to on router navigation. I could not figure out how
// to fix that. So here, we at least memoize the rendering of the whole
// page, so that we don't rerun expensive rendering.
const content = useMemo(() => (
<Root nav={nav(data)}>
<React.Fragment key={counter.current}>{render(data)}</React.Fragment>
</Root>
), [render, nav, data]);

return (
<UserProvider data={userData?.currentUser}>
<Root nav={nav(data)}>
<React.Fragment key={counter.current}>{render(data)}</React.Fragment>
</Root>
{content}
</UserProvider>
);
};
7 changes: 4 additions & 3 deletions frontend/src/layout/header/Search.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { HiOutlineSearch } from "react-icons/hi";
import { ProtoButton, screenWidthAtMost } from "@opencast/appkit";
import { LuX } from "react-icons/lu";

import { useRouter } from "../../router";
import { useRouter, useRouterState } from "../../router";
import {
handleCancelSearch,
SearchRoute,
Expand All @@ -27,6 +27,7 @@ type SearchFieldProps = {
export const SearchField: React.FC<SearchFieldProps> = ({ variant }) => {
const { t } = useTranslation();
const router = useRouter();
const { isTransitioning } = useRouterState();
const ref = useRef<HTMLInputElement>(null);

// If the user is unknown, then we are still in the initial loading phase.
Expand Down Expand Up @@ -177,11 +178,11 @@ export const SearchField: React.FC<SearchFieldProps> = ({ variant }) => {
/>
</label>
</form>
{router.isTransitioning && isSearchActive() && <Spinner
{isTransitioning && isSearchActive() && <Spinner
size={spinnerSize}
css={iconStyle}
/>}
{!router.isTransitioning && isSearchActive() && <ProtoButton
{!isTransitioning && isSearchActive() && <ProtoButton
onClick={() => handleCancelSearch(router, ref)}
css={{
":hover, :focus": {
Expand Down
53 changes: 37 additions & 16 deletions frontend/src/rauta.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, { useEffect, useRef, useState, useTransition } from "react";
import React, { useCallback, useEffect, useMemo, useRef, useState, useTransition } from "react";
import { bug } from "@opencast/appkit";


Expand Down Expand Up @@ -81,6 +81,9 @@ export type RouterLib = {
/** Hook to obtain a reference to the router. */
useRouter: () => RouterControl;

/** Hook to obtain the router state. */
useRouterState: () => RouterState;

/**
* An internal link, using the defined routes. Should be used instead of
* `<a>`. Has to be mounted below a `<Router>`!
Expand Down Expand Up @@ -177,18 +180,20 @@ export interface RouterControl {
*/
listenBeforeNav(listener: BeforeNavListener): () => void;

/**
* Indicates whether we are currently transitioning to a new route. Intended
* to show a loading indicator.
*/
isTransitioning: boolean;

/**
* Indicates whether a user navigated to the current route from outside Tobira.
*/
internalOrigin: boolean;
}

export type RouterState = {
/**
* Indicates whether we are currently transitioning to a new route. Intended
* to show a loading indicator.
*/
isTransitioning: boolean;
};

export const makeRouter = <C extends Config, >(config: C): RouterLib => {
// Helper to log debug messages if `config.debug` is true.
const debugLog = (...args: unknown[]) => {
Expand Down Expand Up @@ -254,7 +259,6 @@ export const makeRouter = <C extends Config, >(config: C): RouterLib => {
}

return {
isTransitioning: context.isTransitioning,
push,
replace,
listenAtNav: (listener: AtNavListener) =>
Expand Down Expand Up @@ -340,12 +344,24 @@ export const makeRouter = <C extends Config, >(config: C): RouterLib => {
atNav: Listeners<AtNavListener>;
beforeNav: Listeners<BeforeNavListener>;
};
isTransitioning: boolean;
};

const Context = React.createContext<ContextData | null>(null);

type StateContextData = {
isTransitioning: boolean;
};
const StateContext = React.createContext<StateContextData | null>(null);

const useRouter = (): RouterControl => useRouterImpl("`useRouter`");
const useRouterState = (): RouterState => {
const context = React.useContext(StateContext);
if (context === null) {
return bug("useRouterState used without a parent <Router>! That's not allowed.");
}

return context;
};

/** Provides the required context for `<Link>` and `<ActiveRoute>` components. */
const Router = ({ initialRoute, children }: RouterProps) => {
Expand All @@ -364,7 +380,7 @@ export const makeRouter = <C extends Config, >(config: C): RouterLib => {
// `StrictMode` work, as with that, this component might be unmounted
// for reasons other than a route change.
const navigatedAway = useRef(false);
const setActiveRoute = (newRoute: ActiveRoute) => {
const setActiveRoute = useCallback((newRoute: ActiveRoute) => {
navigatedAway.current = true;
startTransition(() => {
setActiveRouteRaw(() => newRoute);
Expand All @@ -373,7 +389,7 @@ export const makeRouter = <C extends Config, >(config: C): RouterLib => {
newRoute: newRoute.route.route,
}]);
});
};
}, [navigatedAway, setActiveRouteRaw, listeners]);

// Register some event listeners and set global values.
useEffect(() => {
Expand Down Expand Up @@ -466,14 +482,17 @@ export const makeRouter = <C extends Config, >(config: C): RouterLib => {
}
}, [activeRoute, navigatedAway]);

const contextData = {
const contextData = useMemo(() => ({
setActiveRoute,
activeRoute,
listeners: listeners.current,
isTransitioning: isPending,
};
}), [activeRoute, setActiveRoute, listeners]);

return <Context.Provider value={contextData}>{children}</Context.Provider>;
return <Context.Provider value={contextData}>
<StateContext.Provider value={{ isTransitioning: isPending }}>
{children}
</StateContext.Provider>
</Context.Provider>;
};

const ActiveRoute = () => {
Expand All @@ -490,14 +509,16 @@ export const makeRouter = <C extends Config, >(config: C): RouterLib => {
}
}, [context.activeRoute]);

return context.activeRoute.route.matchedRoute.render();
// Rendered via JSX, as just calling `render()` causes unnecessary rerenders
return <context.activeRoute.route.matchedRoute.render />;
};

return {
Link,
matchRoute,
matchInitialRoute,
useRouter,
useRouterState,
ActiveRoute,
Router,
};
Expand Down
3 changes: 2 additions & 1 deletion frontend/src/router.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ const {
matchRoute,
Router,
useRouter,
useRouterState,
} = makeRouter({
fallback: NotFoundRoute,
routes: [
Expand Down Expand Up @@ -72,7 +73,7 @@ const {
],
});

export { ActiveRoute, Link, matchInitialRoute, matchRoute, Router, useRouter };
export { ActiveRoute, Link, matchInitialRoute, matchRoute, Router, useRouter, useRouterState };

type LinkProps = {
to: string;
Expand Down
6 changes: 3 additions & 3 deletions frontend/src/ui/LoadingIndicator.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,14 @@ import { useRef } from "react";
import { Transition } from "react-transition-group";
import { match } from "@opencast/appkit";

import { useRouter } from "../router";
import { isSearchActive } from "../routes/Search";
import { useRouterState } from "../router";
import { COLORS } from "../color";


/** A thin colored line at the top of the page indicating a page load */
export const LoadingIndicator: React.FC = () => {
const router = useRouter();
const { isTransitioning } = useRouterState();
const ref = useRef<HTMLDivElement>(null);

// If search is active, there is a loading indicator next to the search input.
Expand All @@ -21,7 +21,7 @@ export const LoadingIndicator: React.FC = () => {
const EXIT_DURATION = 150;

// TODO: maybe disable this for `prefers-reduced-motion: reduce`
return <Transition nodeRef={ref} in={router.isTransitioning} timeout={EXIT_DURATION}>{state => (
return <Transition nodeRef={ref} in={isTransitioning} timeout={EXIT_DURATION}>{state => (
<div ref={ref} css={{
position: "fixed",
zIndex: 2000,
Expand Down

0 comments on commit e5af633

Please sign in to comment.