From 40594309b3bf71ac9e4dbd9afaacd6d9ca1cdfe9 Mon Sep 17 00:00:00 2001 From: Lukas Kalbertodt Date: Mon, 18 Nov 2024 16:06:56 +0100 Subject: [PATCH] Get rid of `AuthenticatedDataContext` by using relay store So, quick recap of what was done before this commit: - Initial GQL query can include credentials to immediately unlock `authorizedData`, but lets just consider the route where it doesn't. - `useAuthenticatedDataQuery` was called which sent a request based on whether the `authorizedData` was already present and on whether credentials did exist (by cleverly using `fetchPolicy`). In our case it first did nothing as no credentials are there. - The `authenticatedData` as stored inside a React context, and not in `event`. - The password form, when submitted would send a GQL request. On success the `setAuthData` setter from the context was called, meaning that all components using the contexts were rerendered. Generally nothing terrible going on here, but: `authorizedData` is really a part of the event and having a separate store, instead of just saving and retrieving it from the Relay store feels weird. There was also a bug where only one context was used per realm, which lead to all video blocks showing the same video if one was unlocked. This commit changes this. The context is completely removed and the data is stored in the relay store, making this more idiomatic. But oh boy, it was not easy to get here. Relay docs are just terrible for me to dig through. So one thing seemed clear fairly quickly: the idiomatic way is `useRefetchableFragment` for this. With that, one can only refetch a small part of the initial query, but with potentially different variables. Perfect! Problem is that the `refetch` function's callback function does not get the fetched data. So it is hard to check if the credentials were correct (and to distinguish other network errors). So my solution at the end is to keep the manual `fetchQuery` as that allows us to check the result in a callback. Then, on success, the `refetch` is called with "store-only" to just rerender the components. One of the big time sinks for me was to understand that the manual query we use with `fetchQuery` needs to be exactly the same as what `refetch` will execute. Specifically: use `node()` and not `eventById`. Refetching only works with fragments, so another layer of fragments needed to be introduced. I added a helper function to combine the event with its `authorizedData` for that. This commit is best viewed with whitespace-diff disabled. --- frontend/src/i18n/locales/en.yaml | 1 + frontend/src/routes/Embed.tsx | 32 +-- frontend/src/routes/Realm.tsx | 10 +- frontend/src/routes/Video.tsx | 390 ++++++++++++++++-------------- frontend/src/ui/Blocks/Video.tsx | 26 +- frontend/src/util/index.ts | 33 +-- 6 files changed, 232 insertions(+), 260 deletions(-) diff --git a/frontend/src/i18n/locales/en.yaml b/frontend/src/i18n/locales/en.yaml index 38691a73a..9b0757e6e 100644 --- a/frontend/src/i18n/locales/en.yaml +++ b/frontend/src/i18n/locales/en.yaml @@ -180,6 +180,7 @@ video: password: heading: Protected Video sub-heading: Access to this video is restricted. + no-preview-permission: $t(api-remote-errors.view.event) body: > Please enter the username and the password you have received to access this video; please note that these are not your login credentials. diff --git a/frontend/src/routes/Embed.tsx b/frontend/src/routes/Embed.tsx index b389664a5..93f232d2e 100644 --- a/frontend/src/routes/Embed.tsx +++ b/frontend/src/routes/Embed.tsx @@ -1,4 +1,4 @@ -import { ReactNode, Suspense, useState } from "react"; +import { ReactNode, Suspense } from "react"; import { LuFrown, LuAlertTriangle } from "react-icons/lu"; import { Translation, useTranslation } from "react-i18next"; import { @@ -7,7 +7,7 @@ import { } from "react-relay"; import { unreachable } from "@opencast/appkit"; -import { eventId, getCredentials, isSynced, keyOfId, useAuthenticatedDataQuery } from "../util"; +import { eventId, getCredentials, isSynced, keyOfId } from "../util"; import { GlobalErrorBoundary } from "../util/err"; import { loadQuery } from "../relay"; import { makeRoute, MatchedRoute } from "../rauta"; @@ -19,7 +19,7 @@ import { EmbedQuery } from "./__generated__/EmbedQuery.graphql"; import { EmbedDirectOpencastQuery } from "./__generated__/EmbedDirectOpencastQuery.graphql"; import { EmbedEventData$key } from "./__generated__/EmbedEventData.graphql"; import { PlayerContextProvider } from "../ui/player/PlayerContext"; -import { AuthenticatedDataContext, AuthorizedData, PreviewPlaceholder } from "./Video"; +import { PreviewPlaceholder, useEventWithAuthData } from "./Video"; export const EmbedVideoRoute = makeRoute({ url: ({ videoId }: { videoId: string }) => `/~embed/!v/${keyOfId(videoId)}`, @@ -120,12 +120,8 @@ const embedEventFragment = graphql` endTime duration } - authorizedData(user: $eventUser, password: $eventPassword) { - thumbnail - tracks { uri flavor mimetype resolution isMaster } - captions { uri lang } - segments { uri startTime } - } + ... VideoPageAuthorizedData + @arguments(eventUser: $eventUser, eventPassword: $eventPassword) } } `; @@ -138,12 +134,12 @@ type EmbedProps = { const Embed: React.FC = ({ query, queryRef }) => { const fragmentRef = usePreloadedQuery(query, queryRef); - const event = useFragment( + const protoEvent = useFragment( embedEventFragment, fragmentRef.event, ); + const [event, refetch] = useEventWithAuthData(protoEvent); const { t } = useTranslation(); - const [authenticatedData, setAuthenticatedData] = useState(null); if (!event) { return @@ -170,17 +166,9 @@ const Embed: React.FC = ({ query, queryRef }) => { ; } - const authorizedData = useAuthenticatedDataQuery( - event.id, - event.series?.id, - { authorizedData: event.authorizedData }, - ); - - return authorizedData - ? - : - ; - ; + return event.authorizedData + ? + : ; }; export const BlockEmbedRoute = makeRoute({ diff --git a/frontend/src/routes/Realm.tsx b/frontend/src/routes/Realm.tsx index 306447f2f..d0faed771 100644 --- a/frontend/src/routes/Realm.tsx +++ b/frontend/src/routes/Realm.tsx @@ -27,7 +27,6 @@ import { COLORS } from "../color"; import { useMenu } from "../layout/MenuState"; import { ManageNav } from "./manage"; import { BREAKPOINT as NAV_BREAKPOINT } from "../layout/Navigation"; -import { AuthorizedData, AuthenticatedDataContext } from "./Video"; // eslint-disable-next-line @typescript-eslint/quotes @@ -146,7 +145,6 @@ const RealmPage: React.FC = ({ realm }) => { const { t } = useTranslation(); const siteTitle = useTranslatedConfig(CONFIG.siteTitle); const breadcrumbs = realmBreadcrumbs(t, realm.ancestors); - const [authenticatedData, setAuthenticatedData] = useState(null); const title = realm.isMainRoot ? siteTitle : realm.name; useTitle(title, realm.isMainRoot); @@ -168,11 +166,9 @@ const RealmPage: React.FC = ({ realm }) => { {realm.isUserRealm && } )} - - {realm.blocks.length === 0 && realm.isMainRoot - ? - : } - + {realm.blocks.length === 0 && realm.isMainRoot + ? + : } ; }; diff --git a/frontend/src/routes/Video.tsx b/frontend/src/routes/Video.tsx index 5fb9e06d7..cb99303db 100644 --- a/frontend/src/routes/Video.tsx +++ b/frontend/src/routes/Video.tsx @@ -1,15 +1,14 @@ import React, { - createContext, - Dispatch, ReactElement, ReactNode, - SetStateAction, - useContext, useEffect, useRef, useState, } from "react"; -import { graphql, GraphQLTaggedNode, PreloadedQuery, useFragment } from "react-relay/hooks"; +import { + graphql, GraphQLTaggedNode, PreloadedQuery, RefetchFnDynamic, useFragment, + useRefetchableFragment, +} from "react-relay/hooks"; import { useTranslation } from "react-i18next"; import { fetchQuery, OperationType } from "relay-runtime"; import { @@ -19,7 +18,7 @@ import { QRCodeCanvas } from "qrcode.react"; import { match, unreachable, screenWidthAtMost, screenWidthAbove, useColorScheme, Floating, FloatingContainer, FloatingTrigger, WithTooltip, Card, Button, ProtoButton, - bug, + notNullish, } from "@opencast/appkit"; import { VideoObject, WithContext } from "schema-dts"; @@ -46,7 +45,6 @@ import { keyOfId, playlistId, getCredentials, - useAuthenticatedDataQuery, credentialsStorageKey, } from "../util"; import { BREAKPOINT_SMALL, BREAKPOINT_MEDIUM } from "../GlobalStyle"; @@ -90,9 +88,11 @@ import { PlaylistBlockFromPlaylist } from "../ui/Blocks/Playlist"; import { AuthenticationFormState, FormData, AuthenticationForm } from "./Login"; import { VideoAuthorizedDataQuery, - VideoAuthorizedDataQuery$data, } from "./__generated__/VideoAuthorizedDataQuery.graphql"; import { AuthorizedBlockEvent } from "../ui/Blocks/Video"; +import { + VideoPageAuthorizedData$data, VideoPageAuthorizedData$key, +} from "./__generated__/VideoPageAuthorizedData.graphql"; // =========================================================================================== @@ -122,6 +122,7 @@ export const VideoRoute = makeRoute({ ... UserData event: eventById(id: $id) { ... VideoPageEventData + @arguments(eventUser: $eventUser, eventPassword: $eventPassword) ... on AuthorizedEvent { isReferencedByRealm(path: $realmPath) } @@ -146,11 +147,7 @@ export const VideoRoute = makeRoute({ {... { query, queryRef }} nav={data => data.realm ?