Add IndexedDB offline persistence for document editing#22125
Add IndexedDB offline persistence for document editing#22125
Conversation
24aff7d to
89b12aa
Compare
Deploying openproject with ⚡ PullPreview
|
|
@claude review this pr |
There was a problem hiding this comment.
Pull request overview
This PR enhances offline editing support for collaborative documents by integrating IndexedDB persistence and removing the test-mode local document fallback. The changes require HocuspocusProvider for all scenarios, treating disconnections and timeouts as transitions to offline mode rather than hard connection errors. The editor remains usable during server unavailability with automatic recovery when the connection is restored.
Changes:
- Adds IndexedDB synchronization via
y-indexeddbpackage to persist document state locally - Removes test-mode local document fallback, making HocuspocusProvider required
- Updates feature specs to test offline mode behavior and collaboration notices with proper hocuspocus context
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/package.json | Adds y-indexeddb dependency for local persistence |
| frontend/package-lock.json | Lock file update for y-indexeddb dependency |
| frontend/src/stimulus/controllers/dynamic/documents/init-yjs-provider.controller.ts | Implements IndexedDB persistence initialization and cleanup |
| frontend/src/react/hooks/useCollaboration.ts | Replaces connectionError with offlineMode state, removes local document sync |
| frontend/src/react/OpBlockNoteContainer.tsx | Removes inputField parameter and test-mode document fallback |
| frontend/src/elements/block-note-element.ts | Requires HocuspocusProvider, removes collaboration-disabled branching |
| lib/primer/open_project/forms/block_note_editor.rb | Uses Setting.real_time_text_collaboration_enabled? for collaboration check |
| modules/documents/config/locales/en.yml | Updates connection error message to reflect offline mode |
| modules/documents/app/components/documents/show_edit_view/connection_error_notice_component.html.erb | Adds test selector for offline mode notice |
| modules/documents/app/components/documents/show_edit_view/collaboration_disabled_notice_component.html.erb | Adds test selector for collaboration disabled notice |
| modules/documents/spec/features/real_time_collaboration_spec.rb | Adds offline mode test, restructures hocuspocus context |
| modules/documents/spec/features/documents/project/show_edit_document_spec.rb | Removes allow_any_instance stub |
| modules/documents/spec/features/block_note_editor_spec.rb | Removes allow_any_instance stub, adds collaboration disabled test |
| modules/documents/spec/features/attachment_upload_spec.rb | Removes allow_any_instance stub |
Files not reviewed (1)
- frontend/package-lock.json: Language not supported
| ); | ||
|
|
||
| return Promise.race([ | ||
| persistence.whenSynced.then(() => { |
There was a problem hiding this comment.
💡 (TIL) 💡
- Promise.race()
The Promise.race() static method takes an iterable of promises as input and returns a single Promise. This returned promise settles with the eventual state of the first promise that settles.
- persistence.whenSynced - returns a promise
There was a problem hiding this comment.
Nice solution, TIL as well!
brunopagno
left a comment
There was a problem hiding this comment.
I did not test, but the code looks nice. I'm really glad we're doing this 👏
But just not to leave unsaid, hope you can sanity check if you haven't already a few things:
- How does the indexdb sync with remote state? Is it somewhat stable?
- How about local and remote state sync during active collaboration sessions?
- What happens with readonly when using offline mode? And what happens when the user reconnects?
- Is there a chance that a user might be using offline mode and not notice? I'm afraid it could cause false positives when an admin is setting up hocuspocus, they could think that things are working but it's just offline mode
Still, this is a beautiful job. Thanks for making it 🙇
|
Thanks for the review @brunopagno
Seems fairly stable, but will of course need some real world use. IndexedDB is a local cache only; it doesn't sync with remote directly. Instead, both IndexedDB and Hocuspocus will write into the same shared Y.Doc. On hocuspocus connection Y.js should automatically merge any local changes without conflict. We also set up non-blocking providers- IndexedDB initialization runs in the background with a 10s timeout guard (important because y-indexeddb has no built-in error events), and if it fails the hocuspocus provider continues without offline persistence. It's fairly small in footprint: See: https://github.com/yjs/y-indexeddb/blob/master/src/y-indexeddb.js
Hocuspocus handles all real-time, multi-client sync; IndexedDB is purely a local cache layer that mirrors the Y.Doc on-device. If the connection to sync server (HP) drops, changes are buffered in the Y.Doc locally (including in IndexedDB) and Hocuspocus forwards them automatically on reconnect.
Nice catch, seems we didn't account for this!
No. The connection-error banner will be displayed. See current version below: however, the designs will soon change to this.
|
|
@brunopagno @ihordubas99 as it turns out we needed to revert the non-blocking IndexedDB <-> Hocuspocus provider setup and add local cache presence detection to close two data-loss paths: 🟠 Race condition (users with a local cache) 🔴 No-cache + server offline = block editor
|
06292ce to
a3eaf76
Compare
judithroth
left a comment
There was a problem hiding this comment.
Overall, I like where this is going! There are some details where we should at least discuss and make a conscious decision as I think they affect security.
I am not sure if tests got a bit more flaky or if it is just today. Maybe we should keep an eye on that.
| ); | ||
|
|
||
| return Promise.race([ | ||
| persistence.whenSynced.then(() => { |
There was a problem hiding this comment.
Nice solution, TIL as well!
| return () => document.removeEventListener(PROVIDER_AUTH_ERROR_EVENT, handleProviderAuthError); | ||
| }, []); | ||
| // When offline with no local cache, block the editor entirely to prevent an | ||
| // empty Y.Doc from being synced as the authoritative document on reconnect. |
There was a problem hiding this comment.
Sorry for the very naïve understanding, but isn't the whole point of CRDTs that such a change won't clobber the document?
There was a problem hiding this comment.
but isn't the whole point of CRDTs that such a change won't clobber the document?
This is true- but it requires that the yDOC has some initial "state vector" (akin to git initial commit/ref). And, that initial sync is done from an authoritative data source; in our case hocuspocus onLoadDocument(). As soon as the CRDT achieves this initial state sync- then subsequent conflicts/merges can be resolved. However, IF we start out with an "empty Y.Doc" instance, it would not have any state- and will hence overwrite during onStoreDocument()
See: https://tiptap.dev/docs/hocuspocus/server/hooks#onloaddocument
…are error messages Extract non-IndexedDB refinements from PR #22125 so they can ship independently while IndexedDB offline persistence is evaluated separately. - Gate collaboration on Setting.real_time_text_collaboration_enabled? instead of hardcoding it to true - Remove the test-mode fallback that created a standalone Y.Doc without a provider; HocuspocusProvider is now required for document editing - Refactor useCollaboration hooks: callback-based timeout with proactive cancel on sync, extracted useProviderAuthError hook, JSDoc comments - Add read/write context-aware connection error messages (readonly users see "real-time updates will resume" vs writers see "changes will sync") - Add blocked offline mode: when the server is unreachable and there is no local cache, hide the editor entirely to prevent an empty Y.Doc from being synced as authoritative content on reconnect - Update feature specs to use real hocuspocus shared context instead of stubbing collaboration_enabled, add offline blocking tests
3d85760 to
33717ab
Compare
…are error messages Extract non-IndexedDB refinements from PR #22125 so they can ship independently while IndexedDB offline persistence is evaluated separately. - Gate collaboration on Setting.real_time_text_collaboration_enabled? instead of hardcoding it to true - Remove the test-mode fallback that created a standalone Y.Doc without a provider; HocuspocusProvider is now required for document editing - Refactor useCollaboration hooks: callback-based timeout with proactive cancel on sync, extracted useProviderAuthError hook, JSDoc comments - Add read/write context-aware connection error messages (readonly users see "real-time updates will resume" vs writers see "changes will sync") - Add blocked offline mode: when the server is unreachable and there is no local cache, hide the editor entirely to prevent an empty Y.Doc from being synced as authoritative content on reconnect - Update feature specs to use real hocuspocus shared context instead of stubbing collaboration_enabled, add offline blocking tests
85ade6d to
8272768
Compare
Add y-indexeddb to persist Y.Doc state in the browser's IndexedDB, enabling offline editing with automatic sync on reconnect. - Add IndexeddbPersistence setup with timeout and fallback handling in the Stimulus controller (waitForIndexedDBSync, Promise.race) - Detect cached documents via state vector to distinguish soft offline (has cache, editor editable) from blocking offline (no cache, editor hidden to prevent empty doc overwriting server state) - Clear IndexedDB cache on auth errors to prevent stale data - Add hasCachedDocument prop chain through LiveCollaborationManager, block-note-element, OpBlockNoteContainer, and useCollaboration hook - Add blockingOffline return from useCollaboration for conditional editor visibility - Add feature specs for soft offline mode (write and readonly) and offline-edit-then-reconnect sync
33717ab to
6608537
Compare


Ticket
What are you trying to accomplish?
Add offline editing support for collaborative documents using IndexedDB as a local persistence layer. When a user loses connection to the Hocuspocus server, their edits are stored locally in IndexedDB and automatically synced when the connection is restored.
This PR builds on top of #22564 (collaboration refinements) and adds only the IndexedDB-specific functionality:
hasCachedDocumentprop chain through LiveCollaborationManager, block-note-element, OpBlockNoteContainer, and useCollaboration hookblockingOfflinereturn value from useCollaboration for conditional editor visibilityScreenshots
bn-offline-mode.mp4
Note
This is not the final design — updates will be done separately
What approach did you choose and why?
Uses y-indexeddb to persist the Y.Doc state in the browser's IndexedDB. On page load, the controller waits for IndexedDB sync (with a 10s timeout via Promise.race) before initializing the HocuspocusProvider, so the local state is merged with the server via Y.js CRDTs.
The soft vs blocking offline distinction prevents a fresh empty Y.Doc (no prior cache) from being synced as authoritative content on reconnect — which would overwrite real document content on the server.
Known limitation: IndexedDB data is not encrypted at rest. This is a security concern being evaluated separately and may require a composition wrapper with AES-256-GCM encryption before shipping to production.
Merge checklist