refactor(frontend): extract shared pad-sync module (#106) - #124
Merged
Conversation
viewer-main.js and embed-main.js carried near-duplicate sync loops + lifecycle handlers, and they differed: embed coalesced forced syncs (pendingForcedSync) while the viewer's runSync used a plain in-flight bool that let two forced flushes overlap. Extract one module, src/lib/pad-sync.js (createPadSync): periodic loop with visibility gating, force/keepalive flush with coalescing, and the visibility/pagehide lifecycle wiring. Both entrypoints now use it, so the coalescing (no overlapping forced flushes) is the shared behaviour. The caller injects the request-token getter (viewer/embed read it differently) and tests can inject a fetch. - embed-main.js: drop the local sync state/functions, drive padSync from the host-message handler and run() wiring (semantics unchanged). - viewer-main.js: drop the sync data props + 5 methods; memoise the controller on a non-reactive field via padSync() so it survives the immediate filePath watcher (which runs before created/mounted). Viewer now also coalesces. - New tests/js/lib/pad-sync.test.js (8 cases incl. the coalescing guarantee). vitest 115 green; full Playwright 23/23 on NC 33; committed js/ bundles rebuilt (shared chunk pad-sync-*.chunk.mjs).
Review polish on #106 (non-blocking): the two teardown hooks (beforeDestroy for Vue 2, beforeUnmount for Vue 3) duplicated the flush+stop+remove sequence and went through padSync(), which would lazily create a controller just to tear it down if the viewer was destroyed before its first open. Extract teardownSync(): guard on the existing this._padSync (no controller is spun up only to dispose it) and call it from both hooks. The lazy create stays in the resolve path, which actually needs to sync — the guard belongs at the teardown call site, not in padSync(). No behaviour change for an opened pad. vitest 115 green; full Playwright 23/23.
Every other src/lib/*.js carries the standard SPDX/copyright header; the new module was missing it. Comment-only — the bundled .mjs is byte-identical, only the sourcemap line offsets shift (rebuilt + committed for the stale-js guard).
…fety Review follow-up on #106 (non-blocking safety net): the viewer clears the sync url (configure({ syncUrl: '' })) before switching pads, so a forced flush queued against the previous pad must become a no-op rather than POST to a stale/replaced target. Add a test asserting a coalesced forced replay does not fire once the url is cleared mid-flight (resolves to { status: 'disabled' }). Test only, no runtime change.
There was a problem hiding this comment.
Pull request overview
This PR refactors the frontend pad “sync loop” logic into a single shared module so both the viewer and embed entrypoints use the same lifecycle wiring and forced-sync coalescing behavior, fixing the viewer’s previously possible overlapping forced flushes.
Changes:
- Introduces
createPadSyncinsrc/lib/pad-sync.jsto encapsulate periodic syncing, forced/keepalive flush (with coalescing), andvisibilitychange/pagehidehandlers. - Updates
src/viewer-main.jsandsrc/embed-main.jsto use the shared sync controller (viewer now coalesces forced flushes too). - Adds a focused Vitest suite covering the shared sync behavior and updates built JS artifacts to include the extracted chunk.
Reviewed changes
Copilot reviewed 8 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/js/lib/pad-sync.test.js | Adds unit tests for the extracted sync controller (coalescing, URL handling, lifecycle wiring). |
| src/lib/pad-sync.js | New shared implementation of the sync loop + forced flush coalescing + lifecycle handlers. |
| src/viewer-main.js | Replaces in-component sync state/methods with a memoized createPadSync controller and teardown logic. |
| src/embed-main.js | Removes local sync-loop implementation and delegates syncing/lifecycle behavior to the shared controller. |
| js/pad-sync-B0gz-oBk.chunk.mjs | Generated shared chunk for the new sync module. |
| js/pad-sync-B0gz-oBk.chunk.mjs.map | Source map for the generated shared chunk. |
| js/pad-sync-B0gz-oBk.chunk.mjs.license | License metadata for the generated shared chunk. |
| js/etherpad_nextcloud-viewer-main.mjs | Generated viewer bundle updated to import the shared sync chunk. |
| js/etherpad_nextcloud-viewer-main.mjs.map | Source map updated for the viewer bundle changes. |
| js/etherpad_nextcloud-embed-main.mjs | Generated embed bundle updated to use the shared sync chunk. |
| js/etherpad_nextcloud-embed-main.mjs.map | Source map updated for the embed bundle changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+223
to
+224
| this.padSync().stop() | ||
| this.padSync().configure({ syncUrl: '' }) |
Copilot review on #124 (non-blocking): resolveOpenUrl() reset the sync controller via padSync() at the top, which lazily constructed one just to stop/clear it on the initial immediate filePath watcher (no pad selected yet). Guard the reset on this._padSync; the success path still creates it lazily when there's a pad to actually sync. Mirrors the teardownSync() guard. No behaviour change for an opened pad. vitest 116 green; full Playwright 23/23 on NC 33.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
viewer-main.jsandembed-main.jscarried near-duplicate sync loops + lifecycle handlers, and they behaved differently: embed coalesced forced syncs (pendingForcedSync), while the viewer'srunSyncused a plain in-flight boolean that let two forced flushes overlap.Changes
src/lib/pad-sync.js(createPadSync): the periodic loop (visibility-gated), force/keepalive flush with coalescing (a forced flush arriving mid-request is replayed once, never overlapped), and thevisibilitychange/pagehidewiring. The caller injects the request-token getter (viewer and embed read it from different places) and tests can inject afetchImpl.padSyncfrom the host-message handler andrun()wiring. Behaviour unchanged (embed was already the coalescing one).padSync()so it survives theimmediatefilePathwatcher (which runs before created/mounted). The viewer now coalesces forced syncs too — the fix the issue asked for.Acceptance
Verification
npm run buildcommitted; Vite extracts the sharedpad-sync-*.chunk.mjsimported by both bundles (stale-js guard satisfied).Closes #106.