From a2f037531a3819f0871b7e4e4214e56d4d31f585 Mon Sep 17 00:00:00 2001 From: Tom Moor Date: Thu, 5 Oct 2023 19:50:59 -0400 Subject: [PATCH 1/2] Add registering of client-side relationship properties (#5936) --- app/components/WebsocketProvider.tsx | 4 +- app/models/Comment.ts | 6 +++ app/models/Notification.ts | 4 +- app/models/base/Model.ts | 4 +- app/models/decorators/Field.ts | 9 +++-- app/models/decorators/Relation.ts | 58 +++++++++++++++++++++++++++- app/stores/base/Store.ts | 21 ++++++++++ 7 files changed, 94 insertions(+), 12 deletions(-) diff --git a/app/components/WebsocketProvider.tsx b/app/components/WebsocketProvider.tsx index f12495dec..0d6ca28a4 100644 --- a/app/components/WebsocketProvider.tsx +++ b/app/components/WebsocketProvider.tsx @@ -254,9 +254,7 @@ class WebsocketProvider extends React.Component { }); this.socket.on("comments.delete", (event: WebsocketEntityDeletedEvent) => { - comments.inThread(event.modelId).forEach((comment) => { - comments.remove(comment.id); - }); + comments.remove(event.modelId); }); this.socket.on("groups.create", (event: PartialWithId) => { diff --git a/app/models/Comment.ts b/app/models/Comment.ts index 5e3707886..bfcda0cfa 100644 --- a/app/models/Comment.ts +++ b/app/models/Comment.ts @@ -34,6 +34,12 @@ class Comment extends Model { @observable parentCommentId: string; + /** + * The comment that this comment is a reply to. + */ + @Relation(() => Comment, { onDelete: "cascade" }) + parentComment?: Comment; + /** * The document to which this comment belongs. */ diff --git a/app/models/Notification.ts b/app/models/Notification.ts index a3a27e2e4..e4a99b674 100644 --- a/app/models/Notification.ts +++ b/app/models/Notification.ts @@ -47,7 +47,7 @@ class Notification extends Model { /** * The document that the notification is associated with. */ - @Relation(() => Document) + @Relation(() => Document, { onDelete: "cascade" }) document?: Document; /** @@ -58,7 +58,7 @@ class Notification extends Model { /** * The comment that the notification is associated with. */ - @Relation(() => Comment) + @Relation(() => Comment, { onDelete: "cascade" }) comment?: Comment; /** diff --git a/app/models/base/Model.ts b/app/models/base/Model.ts index dba1635c4..147fac3c2 100644 --- a/app/models/base/Model.ts +++ b/app/models/base/Model.ts @@ -95,12 +95,12 @@ export default abstract class Model { */ toAPI = (): Record => { const fields = getFieldsForModel(this); - return pick(this, fields) || []; + return pick(this, fields); }; /** * Returns a plain object representation of all the properties on the model - * overrides the inbuilt toJSON method to avoid attempting to serialize store + * overrides the native toJSON method to avoid attempting to serialize store * * @returns {Record} */ diff --git a/app/models/decorators/Field.ts b/app/models/decorators/Field.ts index f62e57c0a..5163b7851 100644 --- a/app/models/decorators/Field.ts +++ b/app/models/decorators/Field.ts @@ -1,9 +1,9 @@ import type Model from "../base/Model"; -const fields = new Map(); +const fields = new Map(); export const getFieldsForModel = (target: Model) => - fields.get(target.constructor.name); + fields.get(target.constructor.name) ?? []; /** * A decorator that records this key as a serializable field on the model. @@ -14,7 +14,10 @@ export const getFieldsForModel = (target: Model) => */ const Field = (target: any, propertyKey: keyof T) => { const className = target.constructor.name; - fields.set(className, [...(fields.get(className) || []), propertyKey]); + fields.set(className, [ + ...(fields.get(className) || []), + propertyKey as string, + ]); }; export default Field; diff --git a/app/models/decorators/Relation.ts b/app/models/decorators/Relation.ts index adad623e1..ec8dec8b7 100644 --- a/app/models/decorators/Relation.ts +++ b/app/models/decorators/Relation.ts @@ -4,6 +4,47 @@ import type Model from "../base/Model"; type RelationOptions = { /** Whether this relation is required */ required?: boolean; + /** Behavior of relationship on deletion */ + onDelete: "cascade" | "null" | "ignore"; +}; + +type RelationProperties = { + /** The name of the property on the model that stores the ID of the relation */ + idKey: string; + /** A function that returns the class of the relation */ + relationClassResolver: () => typeof Model; + /** Options for the relation */ + options: RelationOptions; +}; + +type InverseRelationProperties = RelationProperties & { + /** The name of the model class that owns this relation */ + modelName: string; +}; + +const relations = new Map>(new Map()); + +/** + * Returns the inverse relation properties for the given model class. + * + * @param targetClass The model class to get inverse relations for. + * @returns A map of inverse relation properties keyed by the property name. + */ +export const getInverseRelationsForModelClass = (targetClass: typeof Model) => { + const inverseRelations = new Map(); + + relations.forEach((relation, modelName) => { + relation.forEach((properties, propertyName) => { + if (properties.relationClassResolver().name === targetClass.name) { + inverseRelations.set(propertyName, { + ...properties, + modelName, + }); + } + }); + }); + + return inverseRelations; }; /** @@ -20,8 +61,19 @@ export default function Relation( ) { return function (target: any, propertyKey: string) { const idKey = `${String(propertyKey)}Id`; - const relationClass = classResolver(); - const relationClassName = relationClass.name; + + // If the relation has options provided then register them in a map for later lookup. We can use + // this to determine how to update relations when a model is deleted. + if (options) { + const configForClass = + relations.get(target.constructor.name) || new Map(); + configForClass.set(propertyKey, { + options, + relationClassResolver: classResolver, + idKey, + }); + relations.set(target.constructor.name, configForClass); + } Object.defineProperty(target, propertyKey, { get() { @@ -31,6 +83,7 @@ export default function Relation( return undefined; } + const relationClassName = classResolver().name; const store = this.store.rootStore[`${relationClassName.toLowerCase()}s`]; invariant(store, `Store for ${relationClassName} not found`); @@ -41,6 +94,7 @@ export default function Relation( this[idKey] = newValue ? newValue.id : undefined; if (newValue) { + const relationClassName = classResolver().name; const store = this.store.rootStore[`${relationClassName.toLowerCase()}s`]; invariant(store, `Store for ${relationClassName} not found`); diff --git a/app/stores/base/Store.ts b/app/stores/base/Store.ts index dcc305b53..e8ac79808 100644 --- a/app/stores/base/Store.ts +++ b/app/stores/base/Store.ts @@ -6,6 +6,7 @@ import { Class } from "utility-types"; import RootStore from "~/stores/RootStore"; import Policy from "~/models/Policy"; import Model from "~/models/base/Model"; +import { getInverseRelationsForModelClass } from "~/models/decorators/Relation"; import { PaginationParams, PartialWithId } from "~/types"; import { client } from "~/utils/ApiClient"; import { AuthorizationError, NotFoundError } from "~/utils/errors"; @@ -99,6 +100,26 @@ export default abstract class Store { @action remove(id: string): void { + const inverseRelations = getInverseRelationsForModelClass(this.model); + + inverseRelations.forEach((relation) => { + // TODO: Need a better way to get the store for a given model name. + const store = this.rootStore[`${relation.modelName.toLowerCase()}s`]; + const items = store.orderedData.filter( + (item: Model) => item[relation.idKey] === id + ); + + if (relation.options.onDelete === "cascade") { + items.forEach((item: Model) => store.remove(item.id)); + } + + if (relation.options.onDelete === "null") { + items.forEach((item: Model) => { + item[relation.idKey] = null; + }); + } + }); + this.data.delete(id); } From eb71a8f9339dc2f566d96deb7f7a7a0a772dc5a6 Mon Sep 17 00:00:00 2001 From: Tom Moor Date: Thu, 5 Oct 2023 20:01:27 -0400 Subject: [PATCH 2/2] fix: Various compounding memory leaks in editor (#5950) --- app/editor/index.tsx | 40 ++++++++--------- package.json | 2 +- shared/editor/components/Image.tsx | 2 +- .../components/hooks/useComponentSize.ts | 44 +++++++++---------- yarn.lock | 8 ++-- 5 files changed, 46 insertions(+), 50 deletions(-) diff --git a/app/editor/index.tsx b/app/editor/index.tsx index f060aee2c..4ef5770dc 100644 --- a/app/editor/index.tsx +++ b/app/editor/index.tsx @@ -302,6 +302,7 @@ export class Editor extends React.PureComponent< public componentWillUnmount(): void { window.removeEventListener("theme-changed", this.dispatchThemeChanged); + this.view.destroy(); this.mutationObserver?.disconnect(); } @@ -349,27 +350,26 @@ export class Editor extends React.PureComponent< private createNodeViews() { return this.extensions.extensions .filter((extension: ReactNode) => extension.component) - .reduce((nodeViews, extension: ReactNode) => { - const nodeView = ( - node: ProsemirrorNode, - view: EditorView, - getPos: () => number, - decorations: Decoration[] - ) => - new ComponentView(extension.component, { - editor: this, - extension, - node, - view, - getPos, - decorations, - }); - - return { + .reduce( + (nodeViews, extension: ReactNode) => ({ ...nodeViews, - [extension.name]: nodeView, - }; - }, {}); + [extension.name]: ( + node: ProsemirrorNode, + view: EditorView, + getPos: () => number, + decorations: Decoration[] + ) => + new ComponentView(extension.component, { + editor: this, + extension, + node, + view, + getPos, + decorations, + }), + }), + {} + ); } private createCommands() { diff --git a/package.json b/package.json index 86871be8a..fa43b3532 100644 --- a/package.json +++ b/package.json @@ -171,7 +171,7 @@ "prosemirror-state": "^1.4.3", "prosemirror-tables": "^1.3.2", "prosemirror-transform": "^1.7.3", - "prosemirror-view": "^1.31.3", + "prosemirror-view": "^1.32.0", "query-string": "^7.1.3", "quoted-printable": "^1.0.1", "randomstring": "1.2.3", diff --git a/shared/editor/components/Image.tsx b/shared/editor/components/Image.tsx index 8f7a83a6e..da8445b92 100644 --- a/shared/editor/components/Image.tsx +++ b/shared/editor/components/Image.tsx @@ -29,10 +29,10 @@ const Image = (props: Props) => { const [loaded, setLoaded] = React.useState(false); const [naturalWidth, setNaturalWidth] = React.useState(node.attrs.width); const [naturalHeight, setNaturalHeight] = React.useState(node.attrs.height); - const documentBounds = useComponentSize(props.view.dom); const containerBounds = useComponentSize( document.body.querySelector("#full-width-container") ); + const documentBounds = props.view.dom.getBoundingClientRect(); const maxWidth = layoutClass ? documentBounds.width / 3 : documentBounds.width; diff --git a/shared/editor/components/hooks/useComponentSize.ts b/shared/editor/components/hooks/useComponentSize.ts index c03a87a5f..818fd7207 100644 --- a/shared/editor/components/hooks/useComponentSize.ts +++ b/shared/editor/components/hooks/useComponentSize.ts @@ -14,48 +14,44 @@ const defaultRect = { export default function useComponentSize( element: HTMLElement | null ): DOMRect | typeof defaultRect { - const [size, setSize] = useState(element?.getBoundingClientRect()); + const [size, setSize] = useState(() => element?.getBoundingClientRect()); useEffect(() => { - const sizeObserver = new ResizeObserver((entries) => { - entries.forEach(({ target }) => { - const rect = target?.getBoundingClientRect(); - setSize((state) => - state?.width === rect?.width && - state?.height === rect?.height && - state?.x === rect?.x && - state?.y === rect?.y - ? state - : rect - ); - }); + const sizeObserver = new ResizeObserver(() => { + element?.dispatchEvent(new CustomEvent("resize")); }); - if (element) { sizeObserver.observe(element); } - return () => sizeObserver.disconnect(); }, [element]); useEffect(() => { const handleResize = () => { - const rect = element?.getBoundingClientRect(); - setSize((state) => - state?.width === rect?.width && - state?.height === rect?.height && - state?.x === rect?.x && - state?.y === rect?.y - ? state - : rect - ); + setSize((state: DOMRect) => { + const rect = element?.getBoundingClientRect(); + + if ( + rect && + Math.round(state.width) === Math.round(rect.width) && + Math.round(state.height) === Math.round(rect.height) && + Math.round(state.x) === Math.round(rect.x) && + Math.round(state.y) === Math.round(rect.y) + ) { + return state; + } + return rect; + }); }; + window.addEventListener("click", handleResize); window.addEventListener("resize", handleResize); + element?.addEventListener("resize", handleResize); return () => { window.removeEventListener("click", handleResize); window.removeEventListener("resize", handleResize); + element?.removeEventListener("resize", handleResize); }; }); diff --git a/yarn.lock b/yarn.lock index c1e022fbf..397ba0b33 100644 --- a/yarn.lock +++ b/yarn.lock @@ -10848,10 +10848,10 @@ prosemirror-transform@^1.0.0, prosemirror-transform@^1.1.0, prosemirror-transfor dependencies: prosemirror-model "^1.0.0" -prosemirror-view@^1.0.0, prosemirror-view@^1.1.0, prosemirror-view@^1.13.3, prosemirror-view@^1.27.0, prosemirror-view@^1.31.0, prosemirror-view@^1.31.3: - version "1.31.3" - resolved "https://registry.yarnpkg.com/prosemirror-view/-/prosemirror-view-1.31.3.tgz#cfe171c4e50a577526d0235d9ec757cdddf6017d" - integrity sha512-UYDa8WxRFZm0xQLXiPJUVTl6H08Fn0IUVDootA7ZlQwzooqVWnBOXLovJyyTKgws1nprfsPhhlvWgt2jo4ZA6g== +prosemirror-view@^1.0.0, prosemirror-view@^1.1.0, prosemirror-view@^1.13.3, prosemirror-view@^1.27.0, prosemirror-view@^1.31.0, prosemirror-view@^1.32.0: + version "1.32.0" + resolved "https://registry.yarnpkg.com/prosemirror-view/-/prosemirror-view-1.32.0.tgz#2022538d02932c0901232d1e0430c064f79a8ea2" + integrity sha512-HwW7IWgca6ehiW2PA48H/8yl0TakA0Ms5LgN5Krc97oar7GfjIKE/NocUsLe74Jq4mwyWKUNoBljE8WkXKZwng== dependencies: prosemirror-model "^1.16.0" prosemirror-state "^1.0.0"