From d351296e6cd032ea363b83c1f303184b306f6d90 Mon Sep 17 00:00:00 2001 From: Wieland Lindenthal Date: Fri, 5 Jun 2026 16:35:31 +0200 Subject: [PATCH] Fix CTRL+Z in documents https://community.openproject.org/wp/STC-779 Three independent defects together caused Y.UndoManager state to be lost in the documents module: 1. `useCreateBlockNote(editorParams, [activeUser])` used `[activeUser]` as the recreation key. BlockNote's `useCreateBlockNote(options, deps)` uses `deps` as the sole `useMemo` key (options is intentionally NOT in deps). Since `block-note-element.ts` parses a fresh `activeUser` from the host attribute via `JSON.parse` on every `renderCallback` invocation, any path that re-rendered the React tree handed in a new object reference and rebuilt the editor (and its UndoManager). 2. `LiveCollaborationManager.initializeYjsProvider` was not idempotent. Stimulus's `init-yjs-provider` controller can fire `connect()` a second time without firing `disconnect()` (HMR replay, Turbo morph, parent re-attach). The duplicate call destroyed the live Y.Doc + provider and rebuilt both, remounting the editor and wiping its history. The controller now adopts the existing session via `getCurrentSessionFor(documentName)` instead of constructing a duplicate. 3. The `React.StrictMode` wrap in `block-note-element.ts` caused BlockNoteView to destroy and recreate the ProseMirror view between StrictMode's two dev-mode mounts. `y-prosemirror`'s `yUndoPlugin` destroys the `Y.UndoManager` on view-destroy (removing its `afterTransaction` handler from the Y.Doc), but the plugin state retains the now-dead UndoManager reference. After the second mount the editor reused the destroyed UndoManager, no handler was re-attached, no stack items were recorded, and Ctrl+Z was a no-op. StrictMode is dev-only and incompatible with the current y-prosemirror lifecycle, so it is removed from the BlockNote tree. Verified on release/17.5: typing produces stack items, Ctrl+Z reverts to the previous state, Ctrl+Shift+Z reapplies, and the Y.Doc has exactly one `afterTransaction` observer (the live UndoManager's). Synthetic duplicate `connect()` no longer remounts the editor. Co-Authored-By: Claude Opus 4.7 --- frontend/src/elements/block-note-element.ts | 11 +++++-- .../react/components/OpBlockNoteEditor.tsx | 6 +++- .../documents/init-yjs-provider.controller.ts | 13 +++++++- .../helpers/live-collaboration-helpers.ts | 31 +++++++++++++++++-- 4 files changed, 54 insertions(+), 7 deletions(-) diff --git a/frontend/src/elements/block-note-element.ts b/frontend/src/elements/block-note-element.ts index 6ba9a5e5c2e..91f280db7e1 100644 --- a/frontend/src/elements/block-note-element.ts +++ b/frontend/src/elements/block-note-element.ts @@ -88,9 +88,14 @@ class BlockNoteElement extends HTMLElement { this.reactRoot = createRoot(this.editorMount); this.renderCallback = (provider:HocuspocusProvider) => { - this.reactRoot?.render( - React.createElement(React.StrictMode, null, this.BlockNoteReactContainer(provider)) - ); + // Do NOT wrap in React.StrictMode. StrictMode's dev-mode double-mount causes + // BlockNoteView to destroy and recreate the ProseMirror view between the two mounts. + // y-prosemirror's `yUndoPlugin` destroys the Y.UndoManager on view-destroy (removing + // its `afterTransaction` handler from the Y.Doc), but the plugin's STATE retains the + // now-destroyed UndoManager reference. On the second mount the editor reuses the + // destroyed UndoManager, no `afterTransaction` handler is ever re-attached, no stack + // items are recorded, and Ctrl+Z becomes a no-op. + this.reactRoot?.render(this.BlockNoteReactContainer(provider)); }; LiveCollaborationManager.onReady(this.renderCallback); diff --git a/frontend/src/react/components/OpBlockNoteEditor.tsx b/frontend/src/react/components/OpBlockNoteEditor.tsx index 420745b4caf..0964edb131f 100644 --- a/frontend/src/react/components/OpBlockNoteEditor.tsx +++ b/frontend/src/react/components/OpBlockNoteEditor.tsx @@ -122,7 +122,11 @@ export function OpBlockNoteEditor({ }; }, [hocuspocusProvider, doc, activeUser, localeDictionary, attachmentsEnabled, uploadFile, captureExternalLinks]); - const editor = useCreateBlockNote(editorParams, [activeUser]); + // Create the editor exactly once per mount. `useCreateBlockNote(options, deps)` uses `deps` + // as the sole `useMemo` key — `options` is intentionally NOT in deps. `[activeUser]` rebuilt + // the editor (wiping `Y.UndoManager` history) whenever a fresh `activeUser` reference + // reached this component, e.g. on Stimulus reconnect / Turbo morph. + const editor = useCreateBlockNote(editorParams, []); useInlineWpEvents(editor); type EditorType = typeof editor; const theme = useOpTheme(); diff --git a/frontend/src/stimulus/controllers/dynamic/documents/init-yjs-provider.controller.ts b/frontend/src/stimulus/controllers/dynamic/documents/init-yjs-provider.controller.ts index e935131f9f8..3920327b064 100644 --- a/frontend/src/stimulus/controllers/dynamic/documents/init-yjs-provider.controller.ts +++ b/frontend/src/stimulus/controllers/dynamic/documents/init-yjs-provider.controller.ts @@ -74,6 +74,17 @@ export default class extends Controller { connect():void { this.currentToken = this.tokenPayloadValue; + // If a provider for this document is already live, don't build a duplicate + // — adopt it. Stimulus can fire connect() a second time (HMR replay, Turbo + // morph, parent re-attach) without firing disconnect(); building a fresh + // Y.Doc + provider in that case would destroy the live one and wipe the + // editor's Y.UndoManager mid-session. + const existing = LiveCollaborationManager.getCurrentSessionFor(this.documentNameValue); + if (existing) { + this.ownedProvider = existing.provider; + return; + } + const ydoc:Doc = new Y.Doc(); const provider = new HocuspocusProvider({ url: this.hocuspocusUrlValue, @@ -87,7 +98,7 @@ export default class extends Controller { }, }); - LiveCollaborationManager.initializeYjsProvider(provider, ydoc); + LiveCollaborationManager.initializeYjsProvider(provider, ydoc, this.documentNameValue); this.ownedProvider = provider; if (this.refreshUrlValue && this.tokenExpiresInSecondsValue) { diff --git a/frontend/src/stimulus/helpers/live-collaboration-helpers.ts b/frontend/src/stimulus/helpers/live-collaboration-helpers.ts index d1a5452f2ff..3a3d8e933d0 100644 --- a/frontend/src/stimulus/helpers/live-collaboration-helpers.ts +++ b/frontend/src/stimulus/helpers/live-collaboration-helpers.ts @@ -36,21 +36,47 @@ type Listener = (provider:HocuspocusProvider) => void; class LiveCollaborationManagerClass { yjsProviderInstance:HocuspocusProvider|null = null; yjsDocInstance:Doc|null = null; + private currentDocumentName:string|null = null; private listeners:Listener[] = []; /** - * Initializes the YJS Provider + * Returns the active session for the given document, or null if none. + * + * Used by the init-yjs-provider Stimulus controller to detect that a + * provider for the same document is already live — letting it adopt the + * existing session instead of building a duplicate Y.Doc + provider pair. + * Stimulus can fire `connect()` a second time (HMR replay, Turbo morph) + * without firing `disconnect()`; without this check, the spurious re-init + * would tear down the live Y.Doc and wipe the editor's Y.UndoManager + * history mid-session. + */ + getCurrentSessionFor(documentName:string):{provider:HocuspocusProvider; doc:Doc} | null { + if (this.yjsProviderInstance && this.yjsDocInstance && this.currentDocumentName === documentName) { + return { provider: this.yjsProviderInstance, doc: this.yjsDocInstance }; + } + return null; + } + + /** + * Initializes the YJS Provider for the given document. + * + * Callers SHOULD first check {@link getCurrentSessionFor} and adopt any + * existing session rather than calling this with a fresh provider, since + * this method unconditionally tears down the previous provider/doc. + * * @param provider The provider to use * @param doc The Y.Doc instance to use + * @param documentName Logical identifier of the document being edited * @returns void */ - initializeYjsProvider(provider:HocuspocusProvider, doc:Doc) { + initializeYjsProvider(provider:HocuspocusProvider, doc:Doc, documentName:string) { this.destroyYjsProvider(); this.destroyYjsDoc(); this.yjsProviderInstance = provider; this.yjsDocInstance = doc; + this.currentDocumentName = documentName; this.listeners.forEach((listener) => listener(this.yjsProviderInstance!)); } @@ -82,6 +108,7 @@ class LiveCollaborationManagerClass { private destroy():void { this.destroyYjsProvider(); this.destroyYjsDoc(); + this.currentDocumentName = null; this.listeners = []; }