From 5d498632c63a3d28dfaa65d3f269464bcc63d101 Mon Sep 17 00:00:00 2001 From: Tom Moor Date: Sun, 3 Jul 2022 22:48:50 +0200 Subject: [PATCH] fix: Models are not all removed from local store upon access change (#3729) * fix: Clean data from stores correctly on 401/403 response * Convert DataLoader from class component, remove observables and caching * types --- app/components/SocketProvider.tsx | 16 +- app/scenes/Document/components/DataLoader.tsx | 308 +++++++----------- app/scenes/Document/components/Document.tsx | 18 +- app/scenes/Document/index.tsx | 26 +- app/stores/BaseStore.ts | 3 +- app/stores/CollectionsStore.ts | 3 +- app/utils/ApiClient.ts | 7 +- 7 files changed, 167 insertions(+), 214 deletions(-) diff --git a/app/components/SocketProvider.tsx b/app/components/SocketProvider.tsx index 3d6541c1a4ef..9c18da30200c 100644 --- a/app/components/SocketProvider.tsx +++ b/app/components/SocketProvider.tsx @@ -6,6 +6,7 @@ import * as React from "react"; import io from "socket.io-client"; import RootStore from "~/stores/RootStore"; import withStores from "~/components/withStores"; +import { AuthorizationError, NotFoundError } from "~/utils/errors"; import { getVisibilityListener, getPageVisible } from "~/utils/pageVisibility"; type SocketWithAuthentication = { @@ -154,7 +155,10 @@ class SocketProvider extends React.Component { force: true, }); } catch (err) { - if (err.statusCode === 404 || err.statusCode === 403) { + if ( + err instanceof AuthorizationError || + err instanceof NotFoundError + ) { documents.remove(documentId); return; } @@ -216,7 +220,10 @@ class SocketProvider extends React.Component { force: true, }); } catch (err) { - if (err.statusCode === 404 || err.statusCode === 403) { + if ( + err instanceof AuthorizationError || + err instanceof NotFoundError + ) { documents.removeCollectionDocuments(collectionId); memberships.removeCollectionMemberships(collectionId); collections.remove(collectionId); @@ -245,7 +252,10 @@ class SocketProvider extends React.Component { force: true, }); } catch (err) { - if (err.statusCode === 404 || err.statusCode === 403) { + if ( + err instanceof AuthorizationError || + err instanceof NotFoundError + ) { groups.remove(groupId); } } diff --git a/app/scenes/Document/components/DataLoader.tsx b/app/scenes/Document/components/DataLoader.tsx index 452281e87c51..50f2d657369a 100644 --- a/app/scenes/Document/components/DataLoader.tsx +++ b/app/scenes/Document/components/DataLoader.tsx @@ -1,14 +1,12 @@ -import invariant from "invariant"; -import { observable } from "mobx"; import { observer } from "mobx-react"; import * as React from "react"; -import { RouteComponentProps, StaticContext } from "react-router"; -import RootStore from "~/stores/RootStore"; +import { useLocation, RouteComponentProps, StaticContext } from "react-router"; import Document from "~/models/Document"; import Revision from "~/models/Revision"; import Error404 from "~/scenes/Error404"; import ErrorOffline from "~/scenes/ErrorOffline"; -import withStores from "~/components/withStores"; +import usePolicy from "~/hooks/usePolicy"; +import useStores from "~/hooks/useStores"; import { NavigationNode } from "~/types"; import { NotFoundError, OfflineError } from "~/utils/errors"; import history from "~/utils/history"; @@ -16,146 +14,99 @@ import { matchDocumentEdit } from "~/utils/routeHelpers"; import HideSidebar from "./HideSidebar"; import Loading from "./Loading"; -type Props = RootStore & - RouteComponentProps< - { - documentSlug: string; - revisionId?: string; - shareId?: string; - title?: string; - }, - StaticContext, - { - title?: string; - } - > & { - children: (arg0: any) => React.ReactNode; - }; - -@observer -class DataLoader extends React.Component { - sharedTree: NavigationNode | null | undefined; - - @observable - document: Document | null | undefined; - - @observable - revision: Revision | null | undefined; - - @observable - shapshot: Blob | null | undefined; - - @observable - error: Error | null | undefined; - - componentDidMount() { - const { documents, match } = this.props; - this.document = documents.getByUrl(match.params.documentSlug); - this.sharedTree = this.document - ? documents.getSharedTree(this.document.id) - : undefined; - this.loadDocument(); - } - - componentDidUpdate(prevProps: Props) { - // If we have the document in the store, but not it's policy then we need to - // reload from the server otherwise the UI will not know which authorizations - // the user has - if (this.document) { - const document = this.document; - const policy = this.props.policies.get(document.id); - - if ( - !policy && - !this.error && - this.props.auth.user && - this.props.auth.user.id - ) { - this.loadDocument(); +type Params = { + documentSlug: string; + revisionId?: string; + shareId?: string; +}; + +type LocationState = { + title?: string; + restore?: boolean; + revisionId?: string; +}; + +type Children = (options: { + document: Document; + revision: Revision | undefined; + abilities: Record; + isEditing: boolean; + readOnly: boolean; + onCreateLink: (title: string) => Promise; + sharedTree: NavigationNode | undefined; +}) => React.ReactNode; + +type Props = RouteComponentProps & { + children: Children; +}; + +function DataLoader({ match, children }: Props) { + const { ui, shares, documents, auth, revisions } = useStores(); + const { team } = auth; + const [error, setError] = React.useState(null); + const { revisionId, shareId, documentSlug } = match.params; + const document = documents.getByUrl(match.params.documentSlug); + const revision = revisionId ? revisions.get(revisionId) : undefined; + const sharedTree = document + ? documents.getSharedTree(document.id) + : undefined; + const isEditRoute = match.path === matchDocumentEdit; + const isEditing = isEditRoute || !!auth.team?.collaborativeEditing; + const can = usePolicy(document ? document.id : ""); + const location = useLocation(); + + React.useEffect(() => { + async function fetchDocument() { + try { + await documents.fetchWithSharedTree(documentSlug, { + shareId, + }); + } catch (err) { + setError(err); } } + fetchDocument(); + }, [ui, documents, document, shareId, documentSlug]); - // Also need to load the revision if it changes - const { revisionId } = this.props.match.params; - - if ( - prevProps.match.params.revisionId !== revisionId && - revisionId && - revisionId !== "latest" - ) { - this.loadRevision(); - } - } - - get isEditRoute() { - return this.props.match.path === matchDocumentEdit; - } - - get isEditing() { - return this.isEditRoute || this.props.auth?.team?.collaborativeEditing; - } - - onCreateLink = async (title: string) => { - const document = this.document; - invariant(document, "document must be loaded to create link"); - - const newDocument = await this.props.documents.create({ - collectionId: document.collectionId, - parentDocumentId: document.parentDocumentId, - title, - text: "", - }); - - return newDocument.url; - }; - - loadRevision = async () => { - const { revisionId } = this.props.match.params; - - if (revisionId) { - this.revision = await this.props.revisions.fetch(revisionId); - } - }; - - loadDocument = async () => { - const { shareId, documentSlug, revisionId } = this.props.match.params; - - // sets the document as active in the sidebar if we already have it loaded - if (this.document) { - this.props.ui.setActiveDocument(this.document); - } - - try { - const response = await this.props.documents.fetchWithSharedTree( - documentSlug, - { - shareId, - } - ); - this.sharedTree = response.sharedTree; - this.document = response.document; - + React.useEffect(() => { + async function fetchRevision() { if (revisionId && revisionId !== "latest") { - await this.loadRevision(); - } else { - this.revision = undefined; + try { + await revisions.fetch(revisionId); + } catch (err) { + setError(err); + } } - } catch (err) { - this.error = err; - return; } + fetchRevision(); + }, [revisions, revisionId]); + + const onCreateLink = React.useCallback( + async (title: string) => { + if (!document) { + throw new Error("Document not loaded yet"); + } - const document = this.document; + const newDocument = await documents.create({ + collectionId: document.collectionId, + parentDocumentId: document.parentDocumentId, + title, + text: "", + }); + + return newDocument.url; + }, + [document, documents] + ); + React.useEffect(() => { if (document) { - const can = this.props.policies.abilities(document.id); - // sets the document as active in the sidebar, ideally in the future this - // will be route driven. - this.props.ui.setActiveDocument(document); + // sets the current document as active in the sidebar + ui.setActiveDocument(document); // If we're attempting to update an archived, deleted, or otherwise // uneditable document then forward to the canonical read url. - if (!can.update && this.isEditRoute) { + if (!can.update && isEditRoute) { history.push(document.url); return; } @@ -163,72 +114,51 @@ class DataLoader extends React.Component { // Prevents unauthorized request to load share information for the document // when viewing a public share link if (can.read) { - this.props.shares.fetch(document.id).catch((err) => { + shares.fetch(document.id).catch((err) => { if (!(err instanceof NotFoundError)) { throw err; } }); } } - }; - - render() { - const { location, policies, auth, match, ui } = this.props; - const { revisionId } = match.params; - - if (this.error) { - return this.error instanceof OfflineError ? ( - - ) : ( - - ); - } - - const team = auth.team; - const document = this.document; - const revision = this.revision; - - if (!document || !team || (revisionId && !revision)) { - return ( - <> - - {this.isEditing && !team?.collaborativeEditing && ( - - )} - - ); - } + }, [can.read, can.update, document, isEditRoute, shares, ui]); - const abilities = policies.abilities(document.id); - // We do not want to remount the document when changing from view->edit - // on the multiplayer flag as the doc is guaranteed to be upto date. - const key = team.collaborativeEditing - ? "" - : this.isEditing - ? "editing" - : "read-only"; + if (error) { + return error instanceof OfflineError ? : ; + } + if (!document || !team || (revisionId && !revision)) { return ( - - {this.isEditing && !team.collaborativeEditing && ( - - )} - {this.props.children({ - document, - revision, - abilities, - isEditing: this.isEditing, - readOnly: - !this.isEditing || - !abilities.update || - document.isArchived || - !!revisionId, - onCreateLink: this.onCreateLink, - sharedTree: this.sharedTree, - })} - + <> + + {isEditing && !team?.collaborativeEditing && } + ); } + + // We do not want to remount the document when changing from view->edit + // on the multiplayer flag as the doc is guaranteed to be upto date. + const key = team.collaborativeEditing + ? "" + : isEditing + ? "editing" + : "read-only"; + + return ( + + {isEditing && !team.collaborativeEditing && } + {children({ + document, + revision, + abilities: can, + isEditing, + readOnly: + !isEditing || !can.update || document.isArchived || !!revisionId, + onCreateLink, + sharedTree, + })} + + ); } -export default withStores(DataLoader); +export default observer(DataLoader); diff --git a/app/scenes/Document/components/Document.tsx b/app/scenes/Document/components/Document.tsx index 33f7adbe07f1..9d86f4782cc8 100644 --- a/app/scenes/Document/components/Document.tsx +++ b/app/scenes/Document/components/Document.tsx @@ -55,13 +55,21 @@ import References from "./References"; const AUTOSAVE_DELAY = 3000; +type Params = { + documentSlug: string; + revisionId?: string; + shareId?: string; +}; + +type LocationState = { + title?: string; + restore?: boolean; + revisionId?: string; +}; + type Props = WithTranslation & RootStore & - RouteComponentProps< - Record, - StaticContext, - { restore?: boolean; revisionId?: string } - > & { + RouteComponentProps & { sharedTree?: NavigationNode; abilities: Record; document: Document; diff --git a/app/scenes/Document/index.tsx b/app/scenes/Document/index.tsx index 37c617e037d3..201dd25cb620 100644 --- a/app/scenes/Document/index.tsx +++ b/app/scenes/Document/index.tsx @@ -7,13 +7,21 @@ import DataLoader from "./components/DataLoader"; import Document from "./components/Document"; import SocketPresence from "./components/SocketPresence"; -export default function DocumentScene( - props: RouteComponentProps< - { documentSlug: string; revisionId: string }, - StaticContext, - { title?: string } - > -) { +type Params = { + documentSlug: string; + revisionId?: string; + shareId?: string; +}; + +type LocationState = { + title?: string; + restore?: boolean; + revisionId?: string; +}; + +type Props = RouteComponentProps; + +export default function DocumentScene(props: Props) { const { ui } = useStores(); const team = useCurrentTeam(); const { documentSlug, revisionId } = props.match.params; @@ -47,12 +55,12 @@ export default function DocumentScene( if (isActive && !isMultiplayer) { return ( - + ); } - return ; + return ; }} ); diff --git a/app/stores/BaseStore.ts b/app/stores/BaseStore.ts index 5989fa775cb8..965ef684f521 100644 --- a/app/stores/BaseStore.ts +++ b/app/stores/BaseStore.ts @@ -7,6 +7,7 @@ import BaseModel from "~/models/BaseModel"; import Policy from "~/models/Policy"; import { PaginationParams } from "~/types"; import { client } from "~/utils/ApiClient"; +import { AuthorizationError, NotFoundError } from "~/utils/errors"; type PartialWithId = Partial & { id: string }; @@ -209,7 +210,7 @@ export default abstract class BaseStore { this.addPolicies(res.policies); return this.add(res.data); } catch (err) { - if (err.statusCode === 403) { + if (err instanceof AuthorizationError || err instanceof NotFoundError) { this.remove(id); } diff --git a/app/stores/CollectionsStore.ts b/app/stores/CollectionsStore.ts index 969229a6701d..dd37dcf74a14 100644 --- a/app/stores/CollectionsStore.ts +++ b/app/stores/CollectionsStore.ts @@ -4,6 +4,7 @@ import { computed, action } from "mobx"; import Collection from "~/models/Collection"; import { NavigationNode } from "~/types"; import { client } from "~/utils/ApiClient"; +import { AuthorizationError, NotFoundError } from "~/utils/errors"; import BaseStore from "./BaseStore"; import RootStore from "./RootStore"; @@ -158,7 +159,7 @@ export default class CollectionsStore extends BaseStore { this.addPolicies(res.policies); return this.add(res.data); } catch (err) { - if (err.statusCode === 403) { + if (err instanceof AuthorizationError || err instanceof NotFoundError) { this.remove(id); } diff --git a/app/utils/ApiClient.ts b/app/utils/ApiClient.ts index f3245c3804f2..63eb88baa656 100644 --- a/app/utils/ApiClient.ts +++ b/app/utils/ApiClient.ts @@ -141,16 +141,11 @@ class ApiClient { // Handle failed responses const error: { - statusCode?: number; - response?: Response; message?: string; error?: string; data?: Record; } = {}; - error.statusCode = response.status; - error.response = response; - try { const parsed = await response.json(); error.message = parsed.message || ""; @@ -186,7 +181,7 @@ class ApiClient { throw new ServiceUnavailableError(error.message); } - throw new RequestError(`Error ${error.statusCode}: ${error.message}`); + throw new RequestError(`Error ${response.status}: ${error.message}`); }; get = (