From 7199088d1b344198f49e767e87915162633c29ca Mon Sep 17 00:00:00 2001 From: Tom Moor Date: Wed, 12 Oct 2022 09:19:07 -0400 Subject: [PATCH] fix: Multiplayer changes attributed to incorrect user (#4282) * fix: Multiplayer changes attributed to the wrong user, performance improvements * fix: Actually use _last_ editor --- server/collaboration/LoggerExtension.ts | 6 ++-- server/collaboration/PersistenceExtension.ts | 33 +++++++++++++++++-- .../commands/documentCollaborativeUpdater.ts | 11 ++++++- server/commands/documentUpdater.ts | 2 +- server/logging/Logger.ts | 2 +- server/models/Event.ts | 2 +- 6 files changed, 47 insertions(+), 9 deletions(-) diff --git a/server/collaboration/LoggerExtension.ts b/server/collaboration/LoggerExtension.ts index 6b381e161..18ee06bc7 100644 --- a/server/collaboration/LoggerExtension.ts +++ b/server/collaboration/LoggerExtension.ts @@ -8,17 +8,17 @@ import Logger from "@server/logging/Logger"; export default class LoggerExtension implements Extension { async onLoadDocument(data: onLoadDocumentPayload) { - Logger.info("hocuspocus", `Loaded document "${data.documentName}"`, { + Logger.info("multiplayer", `Loaded document "${data.documentName}"`, { userId: data.context.user?.id, }); } async onConnect(data: onConnectPayload) { - Logger.info("hocuspocus", `New connection to "${data.documentName}"`); + Logger.info("multiplayer", `New connection to "${data.documentName}"`); } async onDisconnect(data: onDisconnectPayload) { - Logger.info("hocuspocus", `Closed connection to "${data.documentName}"`, { + Logger.info("multiplayer", `Closed connection to "${data.documentName}"`, { userId: data.context.user?.id, }); } diff --git a/server/collaboration/PersistenceExtension.ts b/server/collaboration/PersistenceExtension.ts index 3b335e990..8de33cfe0 100644 --- a/server/collaboration/PersistenceExtension.ts +++ b/server/collaboration/PersistenceExtension.ts @@ -1,6 +1,7 @@ import { onStoreDocumentPayload, onLoadDocumentPayload, + onChangePayload, Extension, } from "@hocuspocus/server"; import * as Y from "yjs"; @@ -15,6 +16,12 @@ import markdownToYDoc from "./utils/markdownToYDoc"; spanName: "persistence", }) export default class PersistenceExtension implements Extension { + /** + * Map of documentId -> userIds that have modified the document since it + * was last persisted to the database. The map is cleared on every save. + */ + documentCollaboratorIds = new Map>(); + async onLoadDocument({ documentName, ...data }: onLoadDocumentPayload) { const [, documentId] = documentName.split("."); const fieldName = "default"; @@ -62,19 +69,41 @@ export default class PersistenceExtension implements Extension { }); } + async onChange({ context, documentName }: onChangePayload) { + Logger.debug( + "multiplayer", + `${context.user?.name} changed ${documentName}` + ); + + const state = this.documentCollaboratorIds.get(documentName) ?? new Set(); + state.add(context.user?.id); + this.documentCollaboratorIds.set(documentName, state); + } + async onStoreDocument({ document, context, documentName, }: onStoreDocumentPayload) { const [, documentId] = documentName.split("."); - Logger.info("database", `Persisting ${documentId}`); + + // Find the collaborators that have modified the document since it was last + // persisted and clear the map. + const documentCollaboratorIds = this.documentCollaboratorIds.get( + documentName + ); + const collaboratorIds = documentCollaboratorIds + ? Array.from(documentCollaboratorIds.values()) + : [context.user?.id]; + this.documentCollaboratorIds.delete(documentName); try { await documentCollaborativeUpdater({ documentId, ydoc: document, - userId: context.user?.id, + // TODO: Right now we're attributing all changes to the last editor, + // It would be nice in the future to have multiple editors per revision. + userId: collaboratorIds.pop(), }); } catch (err) { Logger.error("Unable to persist document", err, { diff --git a/server/commands/documentCollaborativeUpdater.ts b/server/commands/documentCollaborativeUpdater.ts index 94274e444..5711a105a 100644 --- a/server/commands/documentCollaborativeUpdater.ts +++ b/server/commands/documentCollaborativeUpdater.ts @@ -4,6 +4,7 @@ import { Node } from "prosemirror-model"; import * as Y from "yjs"; import { sequelize } from "@server/database/sequelize"; import { schema, serializer } from "@server/editor"; +import Logger from "@server/logging/Logger"; import { Document, Event } from "@server/models"; export default async function documentCollaborativeUpdater({ @@ -18,7 +19,10 @@ export default async function documentCollaborativeUpdater({ return sequelize.transaction(async (transaction) => { const document = await Document.unscoped() .scope("withState") - .findByPk(documentId, { + .findOne({ + where: { + id: documentId, + }, transaction, lock: { of: Document, @@ -38,6 +42,11 @@ export default async function documentCollaborativeUpdater({ return; } + Logger.info( + "multiplayer", + `Persisting ${documentId}, attributed to ${userId}` + ); + // extract collaborators from doc user data const pud = new Y.PermanentUserData(ydoc); const pudIds = Array.from(pud.clients.values()); diff --git a/server/commands/documentUpdater.ts b/server/commands/documentUpdater.ts index a49420686..8567d79cf 100644 --- a/server/commands/documentUpdater.ts +++ b/server/commands/documentUpdater.ts @@ -112,7 +112,7 @@ export default async function documentUpdater({ } if (document.title !== previousTitle) { - Event.schedule({ + await Event.schedule({ name: "documents.title_change", documentId: document.id, collectionId: document.collectionId, diff --git a/server/logging/Logger.ts b/server/logging/Logger.ts index b304d7777..21f85a50e 100644 --- a/server/logging/Logger.ts +++ b/server/logging/Logger.ts @@ -11,7 +11,7 @@ const isProduction = env.ENVIRONMENT === "production"; type LogCategory = | "lifecycle" - | "hocuspocus" + | "multiplayer" | "http" | "commands" | "worker" diff --git a/server/models/Event.ts b/server/models/Event.ts index 4e2e7806d..a6419ee06 100644 --- a/server/models/Event.ts +++ b/server/models/Event.ts @@ -103,7 +103,7 @@ class Event extends IdModel { */ static schedule(event: Partial) { const now = new Date(); - globalEventQueue.add( + return globalEventQueue.add( this.build({ createdAt: now, ...event,