Conversation
changed double brackets to single brackets
changed double brackets to single brackets
removed extra indents
Replaced double brackets with single ones
removed extra indents
removed extra indents
removed extra indents after the colon
removed extra indents after the colon
removed extra indents after the colon
…al document handling
…ub.com/ihordubas99/openproject into feature/72373-offline-document-editing
…ment-editing [72373]: Add offline editing with IndexedDB sync support
Since HocuspocusProvider is now available in test environments (backed by IndexedDB and hocuspocus), the separate test-mode code path that created a local Y.Doc is no longer needed. OpBlockNoteContainer now always expects a HocuspocusProvider, removing the optional provider type, the isTest check, and the conditional doc-creation logic. - block-note-element: skip initialization when collaboration is disabled; provider parameter is now required (non-optional type) - OpBlockNoteContainer: remove isTest check, local-doc fallback, inputField/inputText props, and the optional HocuspocusProvider type - useCollaboration: remove optional provider type and doc parameter; always derive doc from provider.document - block_note_editor.rb: drive collaboration_enabled from the real setting instead of hardcoding true - block_note_editor_spec.rb: remove the any_instance stub that forced collaboration off; add a proper spec for the disabled-collaboration case - init-yjs-provider.controller.ts: fix no-misused-promises by extracting the async body of connect() into a private setupProvider() method so the Stimulus lifecycle hook returns void as its type requires; fix no-floating-promises on indexeddbPersistence.destroy() with void operator
akabiru
changed the title
…pecs Add test_selector attributes to the connection error and collaboration disabled Banner components so they can be targeted by feature specs. Update the offline mode test to access the notice via editor.shadow_root rather than page-level have_test_selector, which cannot pierce the Shadow DOM boundary of op-block-note. Also update the connection_error_notice i18n description to reflect offline mode messaging (was: "server is unreachable"). - connection_error_notice_component: add test_selector "connection-error-notice" - collaboration_disabled_notice_component: add test_selector "collaboration-disabled-notice" - block_note_editor_spec: assert collaboration-disabled-notice banner text; use editor.shadow_root + have_css to access connection-error-notice - real_time_collaboration_spec: gate with real_time_text_collaboration_enabled setting; nest hocuspocus tests under dedicated context; move offline mode test here where no hocuspocus server is started Co-authored-by: Ihor Dubas <dubasihor25@gmail.com>
akabiru
force-pushed
the
implementation/72665-remove-test-mode-for-local-document-fallback-require-hocuspocus-provider
branch
from
24aff7d to
89b12aa
Compare
Deploying openproject with ⚡ PullPreview
|
|
@claude review this pr |
frontend/src/stimulus/controllers/dynamic/documents/init-yjs-provider.controller.ts
Show resolved
Hide resolved
Remove the await on waitForIndexedDBSync so the HocuspocusProvider is created immediately rather than waiting up to 10 seconds for IndexedDB to sync. IndexedDB now runs in the background with a .catch() handler. This is safe because: - Y.js CRDTs handle out-of-order merges: updates applied in any order converge to the same result - The Hocuspocus server's onLoadDocument always pushes the authoritative document state to the client on connect (via Y.applyUpdate), so any offline edits IndexedDB applies afterward are forwarded to the server automatically by the provider Also removes the isConnected guard and makes setupProvider synchronous since there are no longer any await points in the function.
- Convert useConnectionTimeout to accept an onTimeout callback instead of returning a boolean state, removing the redundant intermediate useEffect and establishing a single source of truth for offline state - Remove duplicate setHasTimedOut(false) call that was unreachable after the initial reset at the top of the same effect - Register synced/disconnect listeners before checking provider.synced to prevent a race where a sync event fires between the guard check and listener registration
Previously the timeout callback relied on reading provider.synced at fire time to avoid calling onTimeout() after a successful sync. This left the timer running past the point it was needed and made correctness dependent on the provider's live state at an arbitrary future moment. Subscribe to the 'synced' event inside useConnectionTimeout so the timer is cancelled immediately when the provider syncs, and clean up the listener in the effect's teardown.
- Extract INDEXEDDB_SYNC_TIMEOUT_MS to a module-level constant - Fix double-space in IndexedDB sync debug log message - Remove unnecessary fragment wrapper in OpBlockNoteContainer
Document the purpose, behaviour, and state transitions of useConnectionTimeout, useCollaborationProvider, and useCollaboration to make the non-obvious design decisions (register-before-check, proactive cancel on sync, offline-mode semantics) easy to follow.
Move the inline useEffect for the document-level auth error event into its own useProviderAuthError hook, consistent with the callback-based convention used by useConnectionTimeout and useCollaborationProvider. useCollaboration is now a pure orchestrator with no bare effects.
akabiru
changed the title
brunopagno
left a comment
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 
|
modules/documents/app/components/documents/show_edit_view/connection_error_notice_component.rb
Show resolved
Hide resolved
Add a Phase 1 to each soft-offline test that types content and waits for Hocuspocus to persist it (content_binary present) before switching to the unreachable URL. An empty document never triggers a Y.Doc write, so IndexedDB stayed empty and Phase 2 fell into blocking-offline mode instead of soft-offline mode. For the readonly soft-offline test, Phase 1 now logs in as admin to type the seed content (readonly users cannot write), then switches to the readonly user for the offline visit. Also add blocking: param support to ConnectionErrorNoticeComponent and documents controller to display a distinct "server unreachable" message when no local IndexedDB cache exists.
akabiru
force-pushed
the
implementation/72665-remove-test-mode-for-local-document-fallback-require-hocuspocus-provider
branch
from
06292ce to
a3eaf76
Compare
modules/documents/app/components/documents/show_edit_view/connection_error_notice_component.rb
Show resolved
Hide resolved
frontend/src/stimulus/controllers/dynamic/documents/init-yjs-provider.controller.ts
Outdated
Show resolved
Hide resolved
Store the setTimeout ID and call clearTimeout() in the whenSynced .then() callback so the stale timer is cancelled when sync completes before the 10-second deadline, preventing a no-op reject() call.
Ticket
What are you trying to accomplish?
Continues:
Adds two main changes:
Screenshots
bn-offline-mode.mp4
Note
This is not the final design- updates will be done separately
What approach did you choose and why?
As we now have hocuspocus and indexeddb providers available in test environment, we can remove the "hacky" yDoc setup in test and also add more tests! 👍🏾
Merge checklist