From 2d6031ae45ee5656d630b4bd5232d62e93d80aec Mon Sep 17 00:00:00 2001 From: Jannik Stehle Date: Thu, 27 Aug 2026 11:59:21 +0200 Subject: [PATCH 1/4] feat(yjs): improve handling of external file conflicts Disconnect users from a yjs room if they have unsaved changes that would be overwritten by an external update. This ensures they don't lose their unsaved work and can properly save or store it somewhere, before reconnecting to the room via a page reload. Also reflect the disconnected status in the indicator. --- dev/docs/yjs.md | 28 ++- .../components/AppTemplates/AppWrapper.vue | 65 +++--- .../AppTemplates/useAppWrapperYjs.ts | 32 ++- .../src/composables/yjs/useYjsSession.ts | 71 ++++++- .../AppTemplates/AppWrapper.spec.ts | 133 +++++++++++- .../composables/yjs/useYjsSession.spec.ts | 196 +++++++++++++++++- 6 files changed, 464 insertions(+), 61 deletions(-) diff --git a/dev/docs/yjs.md b/dev/docs/yjs.md index 099a60c961c..71409724d85 100644 --- a/dev/docs/yjs.md +++ b/dev/docs/yjs.md @@ -314,10 +314,22 @@ convergence. A peer that joins while `isStale` is already up offers itself the s `nativeEtag` - the observer only fires on change, so without that the room would stay stuck if the elected peer navigated away mid-recovery. +Peers receive the rewrite as ordinary CRDT updates, and a peer holding unsaved work would lose it without a word. So a +peer that is dirty when a _remote_ `isStale` goes up does not follow: it marks itself conflicted, stops its provider +and keeps its own Y.Doc. The editor still shows that work and the user sees a conflict message. A reload rejoins. + +Note that an external write drops _every_ peer in a room that holds any unsaved state, since the dirty state is shared +across all peers. That is deliberate: a peer cannot tell whose edits the recovery would discard, and losing them silently +is worse than a conflict message. + +A peer that is _clean_ follows the recovery, and the commit stamps `savedStateVector` and `lastSavedAt` alongside the +fresh `etag`. + Reset lands before hydrate, so a throw in between leaves every peer looking at an empty document. That path keeps `isStale` up for the next joiner to retry, and locks the session so the editor freezes. Note that the lock does _not_ -block saving - `isDirty` deliberately ignores it so a lock cannot silently discard unsaved work (see Known limits). What -actually stops an empty document reaching disk is `serializeDoc` returning `null` when the adapter reports no content. +block saving - `isDirty` deliberately ignores it so a lock cannot silently discard unsaved work (see Known limits). +What actually stops an empty document reaching disk is `serializeDoc` returning `null` when the adapter reports no +content. ### `_oc_meta` @@ -326,8 +338,8 @@ A `Y.Map` alongside the editor content, used for coordination the editor never s | Key | Written by | Meaning | | ------------------ | ------------------- | --------------------------------------------- | | `etag` | whoever saved last | the etag the room believes is on disk | -| `lastSavedAt` | whoever saved last | fan-out trigger for a peer save | -| `savedStateVector` | whoever saved last | what that peer's doc held when it wrote | +| `lastSavedAt` | last save, recovery | fan-out trigger for a peer save | +| `savedStateVector` | last save, recovery | what that peer's doc held when it wrote | | `isStale` | any writer | the file changed outside this room; rehydrate | | `nativeEtag` | writer that noticed | the etag recovery should settle on | | `recoveryClientId` | writer that noticed | which peer is elected to re-seed the room | @@ -462,10 +474,10 @@ public-link tokens, so any context without a user access token - a public link, `yjsServerUrl`. Editing works, it just does not sync. A signed-in visitor opening someone else's public link is treated the same way, since their token carries no grant on the shared file. -**Connection state is invisible.** The session exposes a `status` ref (`connecting` / `connected` / `disconnected` / -`local`). `AppWrapper` reads it to decide whether a save conflict can be reconciled, but never shows it. A websocket -that drops _after_ a successful sync produces no indicator: the user keeps typing into a Y.Doc that no longer reaches -anyone. Only a connect that never succeeds in the first place is caught, by the timeout above. +**A conflicted peer stays out until reload.** Leaving the room protects unsaved work, but there is no way back inside +the session: the provider is stopped for good, so the user keeps editing a document nobody else sees until they save +elsewhere and reload. Re-joining would mean merging a diverged doc into a room that has moved on, which the CRDT cannot +do meaningfully here. **Conflict reconciliation depends on a timely etag stamp.** While connected, a 409/412 is only retried when the room can account for the write: the fresh etag must match `_oc_meta.etag`, with a short grace period (`ROOM_ETAG_GRACE_MS`) for a diff --git a/packages/web-pkg/src/components/AppTemplates/AppWrapper.vue b/packages/web-pkg/src/components/AppTemplates/AppWrapper.vue index 61af8869660..2d1fb2bd646 100644 --- a/packages/web-pkg/src/components/AppTemplates/AppWrapper.vue +++ b/packages/web-pkg/src/components/AppTemplates/AppWrapper.vue @@ -229,16 +229,25 @@ const { applicationId }) +const isDirty = computed(() => { + if (unref(isReadOnly)) { + return false + } + return unref(currentContent) !== unref(serverContent) +}) + const { session: yjsSession, isSessionReady, effectiveReadOnly, + isConflicted, reconcileConflict } = useAppWrapperYjs({ yjs, applicationId, resource, isReadOnly, + isDirty, loading, loadingError, contentResourceId, @@ -249,7 +258,8 @@ const { fileContentOptions, getFileContents, putFileContents, - applySavedResource + applySavedResource, + onConflict: showExternalUpdateConflict }) // Keep the loading screen up until the Yjs session is synced and hydrated, @@ -261,18 +271,6 @@ const isLoading = computed(() => { return unref(loading) || !unref(isSessionReady) }) -const isDirty = computed(() => { - // Peer edits flow into `currentContent` of a read-only client too, but it - // has nothing to save, so it must never be prompted. Deliberately the - // WebDAV permission and not `effectiveReadOnly`: a session locking mid-edit - // may hold real unsaved work, which must keep the save action and the - // leave guards armed. - if (unref(isReadOnly)) { - return false - } - return unref(currentContent) !== unref(serverContent) -}) - watch(isDirty, (dirty) => { // Prevent reload if there are changes if (dirty) { @@ -516,6 +514,17 @@ const autosavePopup = () => { showMessage({ title: $gettext('File autosaved') }) } +function showExternalUpdateConflict(response: Response = null) { + errorPopup( + new HttpError( + $gettext( + 'This file was updated outside this window. Please copy your changes or save the file under a new name (»Save As...«).' + ), + response + ) + ) +} + /** * Single landing point for a successful write: updates the etag for the next * `If-Match`, the local `resource` ref and the store. `saved` is the PUT's @@ -556,14 +565,7 @@ const saveFileTask = useTask(function* () { if (yield* reconcileConflict(newContent)) { return } - errorPopup( - new HttpError( - $gettext( - 'This file was updated outside this window. Please copy your changes or save the file under a new name (»Save As...«).' - ), - e.response - ) - ) + showExternalUpdateConflict(e.response) return } switch (e.statusCode) { @@ -630,10 +632,11 @@ onMounted(() => { if (editorOptions.autosaveEnabled && !disableAutoSave) { autosaveIntervalId = setInterval( async () => { - if (isDirty.value) { - await save() - autosavePopup() + if (!unref(isDirty) || unref(isConflicted)) { + return } + await save() + autosavePopup() }, (editorOptions.autosaveInterval || 120) * 1000 ) @@ -700,8 +703,18 @@ const downloadFileActionInterceptor = async ( originalAction: Action['handler'] ) => { if (unref(isDirty)) { - await save() - autosavePopup() + if (unref(isConflicted)) { + showMessage({ + title: $gettext('Unsaved changes are not included'), + desc: $gettext( + 'This file was updated outside this window, so the download does not contain your unsaved changes.' + ), + status: 'warning' + }) + } else { + await save() + autosavePopup() + } } originalAction(args) } diff --git a/packages/web-pkg/src/components/AppTemplates/useAppWrapperYjs.ts b/packages/web-pkg/src/components/AppTemplates/useAppWrapperYjs.ts index f84e329c235..01690b90163 100644 --- a/packages/web-pkg/src/components/AppTemplates/useAppWrapperYjs.ts +++ b/packages/web-pkg/src/components/AppTemplates/useAppWrapperYjs.ts @@ -3,7 +3,7 @@ import type { Ref } from 'vue' import { useGettext } from 'vue3-gettext' import { call } from '@opencloud-eu/web-client' import type { Resource } from '@opencloud-eu/web-client' -import { useMessages, useYjsSession } from '../../composables' +import { useMessages, useYjsSession, YjsStatus } from '../../composables' import type { AppFileHandlingResult, FileContentOptions, FileContext } from '../../composables' import type { YjsOptions } from './types' @@ -13,6 +13,8 @@ export interface AppWrapperYjsOptions { applicationId: string resource: Ref isReadOnly: Ref + /** Whether the wrapper holds edits that are not on disk yet. */ + isDirty: Ref loading: Ref loadingError: Ref contentResourceId: Ref @@ -25,6 +27,8 @@ export interface AppWrapperYjsOptions { putFileContents: AppFileHandlingResult['putFileContents'] /** AppWrapper's landing point for a successful write. */ applySavedResource: (etag: string, saved?: Resource | null) => void + /** The file changed outside this window and the session gave up the room. */ + onConflict: () => void } /** @@ -38,6 +42,7 @@ export function useAppWrapperYjs(options: AppWrapperYjsOptions) { applicationId, resource, isReadOnly, + isDirty, loading, loadingError, contentResourceId, @@ -48,7 +53,8 @@ export function useAppWrapperYjs(options: AppWrapperYjsOptions) { fileContentOptions, getFileContents, putFileContents, - applySavedResource + applySavedResource, + onConflict } = options const { $gettext } = useGettext() @@ -85,7 +91,9 @@ export function useAppWrapperYjs(options: AppWrapperYjsOptions) { // saves itself would otherwise read the next reconnect as one and // wipe the room to "recover" it. resource.value = { ...unref(resource), etag: value } - } + }, + hasUnsavedChanges: () => unref(isDirty), + onConflict }) : null @@ -113,7 +121,10 @@ export function useAppWrapperYjs(options: AppWrapperYjsOptions) { // TNext must be `any`: the delegated `call()` generators expect different // resume types, which would otherwise intersect to `never`. function* reconcileConflict(newContent: unknown): Generator { - if (!session || unref(session.status) !== 'connected') return false + if (!session) return false + // Not connected: nothing in the room can account for the write, so this is + // an external change like any other. + if (unref(session.status) !== YjsStatus.Connected) return false try { const fresh = yield* call(getFileContents(currentFileContext, { ...fileContentOptions })) const freshEtag = fresh.headers['OC-ETag'] @@ -129,7 +140,10 @@ export function useAppWrapperYjs(options: AppWrapperYjsOptions) { // republished over: a write from outside the room never reached this // document, and retrying would destroy it unseen. const writtenByRoom = yield* call(session.wasWrittenByRoom(freshEtag)) - if (!writtenByRoom) return false + if (!writtenByRoom) { + session.markConflicted(false) + return false + } // Retry with the merged state, not `newContent`: that snapshot predates // the peer save that caused this conflict, so writing it again would @@ -151,11 +165,15 @@ export function useAppWrapperYjs(options: AppWrapperYjsOptions) { return true } catch (retryErr) { // Fall through to the conflict popup so the user can still recover by - // copying out. + // copying out. The room stays: the cause is just as likely a failed + // request as an external write, and a retry may well go through. console.error('[yjs] conflict reconciliation failed:', retryErr) return false } } - return { session, isSessionReady, effectiveReadOnly, reconcileConflict } + /** True once the session gave up the room over an external write. */ + const isConflicted = computed(() => Boolean(unref(session?.isConflicted))) + + return { session, isSessionReady, effectiveReadOnly, isConflicted, reconcileConflict } } diff --git a/packages/web-pkg/src/composables/yjs/useYjsSession.ts b/packages/web-pkg/src/composables/yjs/useYjsSession.ts index 263f44ad18f..73a96cc7bc5 100644 --- a/packages/web-pkg/src/composables/yjs/useYjsSession.ts +++ b/packages/web-pkg/src/composables/yjs/useYjsSession.ts @@ -46,6 +46,10 @@ export interface YjsSessionOptions { onServerContentChange: (content: string) => void /** A peer save propagated a fresh etag through the room. */ onEtagChange: (etag: string) => void + /** Whether the caller holds edits that are not on disk yet. */ + hasUnsavedChanges: MaybeRefOrGetter + /** The session gave up the room because the file changed outside it. */ + onConflict: () => void } export interface YjsSession { @@ -67,6 +71,18 @@ export interface YjsSession { isLockedForReload: Ref /** Set when the persisted state was stale or Yjs auth failed. */ error: ShallowRef + /** + * True after the session left the room over an external change it cannot + * follow without discarding the caller's unsaved work. The Y.Doc stays, so + * the editor keeps showing that work. + */ + isConflicted: ShallowRef + /** + * Give up the room because the file moved outside it. Idempotent, and a + * no-op in local mode. `notify` off when the caller already surfaced the + * conflict itself. + */ + markConflicted: (notify?: boolean) => void /** * Whether the file now on disk under `etag` was written by this room. * A peer's save is already merged into our Y.Doc, so republishing over it @@ -203,7 +219,9 @@ export function useYjsSession(options: YjsSessionOptions): YjsSession { documentPrefix, onContentChange, onServerContentChange, - onEtagChange + onEtagChange, + hasUnsavedChanges, + onConflict } = options const { $gettext } = useGettext() @@ -217,6 +235,7 @@ export function useYjsSession(options: YjsSessionOptions): YjsSession { const status = shallowRef('connecting') const isReady = shallowRef(false) const isLockedForReload = ref(false) + const isConflicted = shallowRef(false) const error = shallowRef(null) const effectiveReadOnly = computed(() => toValue(isReadOnly) || unref(isLockedForReload)) @@ -391,6 +410,23 @@ export function useYjsSession(options: YjsSessionOptions): YjsSession { } } + /** + * See {@link YjsSession.markConflicted}. Keeps the doc so the user can still + * copy their work out or use "Save As". Deliberately no `error`: the caller + * owns the conflict message and would otherwise get a second, generic toast. + */ + function markConflicted(notify = true) { + if (unref(isConflicted)) return + // Local mode has no room to leave and no peer that could have written the + // file. Conflicting would only stop its autosave, which is the single + // safety net there. + if (unref(status) === YjsStatus.Local) return + isConflicted.value = true + const prov = unref(provider) + if (prov) stopProvider(prov) + if (notify) onConflict() + } + function lockForReload(prov: HocuspocusProvider | null, message: string) { if (unref(isLockedForReload)) return isLockedForReload.value = true @@ -570,22 +606,28 @@ export function useYjsSession(options: YjsSessionOptions): YjsSession { const content = staleRecoveryContent const freshEtag = meta.get('nativeEtag') ?? toValue(resource)?.etag ?? '' - // Three phases, so a crash between reset and hydrate leaves `isStale` set - // and the next joiner retries instead of inheriting an empty doc. None of - // it is a user edit, so none of it is reported as content. + // Reset and hydrate share one transaction: mounted editors must never + // observe the emptied doc, or ProseMirror pads it with an empty paragraph + // that survives the re-seed. A crash inside still commits the reset and + // leaves `isStale` set, so the next joiner retries instead of inheriting + // an empty doc. None of it is a user edit, so none of it is reported as + // content. isRewritingDoc = true try { doc.transact(() => { current.reset?.(doc) + current.hydrate(doc, content) }, STALE_RECOVERY_RESET_ORIGIN) - current.hydrate(doc, content) - doc.transact(() => { meta.delete('isStale') meta.delete('nativeEtag') meta.delete('recoveryClientId') if (freshEtag) meta.set('etag', freshEtag) + // The recovered body is what is on disk now, so clean peers take the + // ordinary peer-save fan-out and stop counting it as unsaved work. + meta.set('savedStateVector', Y.encodeStateVector(doc)) + meta.set('lastSavedAt', Date.now()) }, STALE_RECOVERY_COMMIT_ORIGIN) staleRecoveryContent = null @@ -753,16 +795,16 @@ export function useYjsSession(options: YjsSessionOptions): YjsSession { // observer; everything in it belongs to `runInitialHydration`, which // reads the same keys with the context to act on them. const isMidSession = unref(isReady) - // Remote ops carry no string origin, so this means "a peer is saving + // Remote ops carry no string origin, so this means "a peer is acting // right now" - not the initial sync, not one of our own writes. - const isPeerSave = isMidSession && !OWN_META_ORIGINS.includes(transaction.origin) + const isRemoteMetaWrite = isMidSession && !OWN_META_ORIGINS.includes(transaction.origin) // Peer-save fan-out. The fresh etag keeps our next If-Match correct. // The content only follows when the peer's snapshot covers everything // we hold; otherwise our dirty flag would drop over edits that never // reached the peer's PUT and they could leave with the tab. Re-checked // after serializing because a keystroke can land while that runs. - if (isPeerSave) { + if (isRemoteMetaWrite) { if (event.keysChanged.has('etag')) { const newEtag = meta.get('etag') if (newEtag) onEtagChange(newEtag) @@ -796,6 +838,14 @@ export function useYjsSession(options: YjsSessionOptions): YjsSession { // Stale-state signal: every peer sees it, only the claimed one gets // past the guards in `recoverFromStaleState`. if (event.keysChanged.has('isStale') && meta.get('isStale') === true) { + // A peer is about to wipe and re-seed the room. Following that would + // silently drop our unsaved edits, so leave the room and keep the + // local doc instead. Our own `flagStale` (raised while `isReady` is + // still false) and the initial sync replay stay on the recovery path. + if (isRemoteMetaWrite && toValue(hasUnsavedChanges)) { + markConflicted() + return + } void recoverFromStaleState(doc, prov) } } @@ -814,6 +864,7 @@ export function useYjsSession(options: YjsSessionOptions): YjsSession { // would drop the loading screen while `ydoc` is already null. error.value = null isLockedForReload.value = false + isConflicted.value = false isReady.value = false hasLocalOnlyContent = false staleRecoveryContent = null @@ -911,6 +962,8 @@ export function useYjsSession(options: YjsSessionOptions): YjsSession { status, isReady, isLockedForReload, + isConflicted, + markConflicted, error, wasWrittenByRoom, beginSave, diff --git a/packages/web-pkg/tests/unit/components/AppTemplates/AppWrapper.spec.ts b/packages/web-pkg/tests/unit/components/AppTemplates/AppWrapper.spec.ts index 63d5261b9d4..2f437841d57 100644 --- a/packages/web-pkg/tests/unit/components/AppTemplates/AppWrapper.spec.ts +++ b/packages/web-pkg/tests/unit/components/AppTemplates/AppWrapper.spec.ts @@ -31,7 +31,10 @@ vi.mock('../../../../src/composables/appDefaults/useAppDefaults', () => ({ useAppDefaults: (...args: unknown[]) => useAppDefaultsSpy(...args) })) -vi.mock('../../../../src/composables/yjs/useYjsSession', () => ({ +// Spread the original: `YjsStatus` is a real const the wrapper compares +// against, not just a type. +vi.mock('../../../../src/composables/yjs/useYjsSession', async (importOriginal) => ({ + ...(await importOriginal()), useYjsSession: (...args: unknown[]) => useYjsSessionSpy(...args) })) @@ -48,6 +51,7 @@ beforeEach(() => { afterEach(() => { consoleErrorSpy.mockRestore() + vi.useRealTimers() }) const FILE_A = mock({ @@ -81,16 +85,25 @@ function setup({ const beginSave = vi.fn() const serializeMerged = vi.fn().mockResolvedValue(mergedContent) const isLockedForReload = ref(false) + const isConflicted = ref(false) + const markConflicted = vi.fn(() => { + isConflicted.value = true + }) const statusRef = ref(status) const currentFileContext = ref(mock({ space: mock(), path: '/a.md' })) // Deferred so the test controls when each half of the load completes. let resolveInfo: (r: Resource) => void let resolveContents: (c: GetFileContentsResponse) => void + let rejectContents: (e: Error) => void const getFileInfo = vi.fn().mockImplementation(() => new Promise((r) => (resolveInfo = r))) - const getFileContents = vi - .fn() - .mockImplementation(() => new Promise((r) => (resolveContents = r))) + const getFileContents = vi.fn().mockImplementation( + () => + new Promise((resolve, reject) => { + resolveContents = resolve + rejectContents = reject + }) + ) const closeApp = vi.fn() useAppDefaultsSpy.mockReturnValue( @@ -112,6 +125,8 @@ function setup({ // Auto-mocked refs are truthy, which would fold into `effectiveReadOnly` // and hard-wire `isDirty` to false. isLockedForReload: isLockedForReload as any, + isConflicted: isConflicted as any, + markConflicted, error: ref(null) as any, wasWrittenByRoom, beginSave, @@ -162,6 +177,8 @@ function setup({ currentFileContext, isEnabled: () => sessionOptions?.enabled(), isLockedForReload, + isConflicted, + markConflicted, putFileContents, closeApp, getFileContents, @@ -189,6 +206,9 @@ function setup({ mock({ body, headers: { 'OC-ETag': 'etag' } as any }) ) await flushPromises() + }, + rejectContent(error: Error) { + rejectContents(error) } } } @@ -380,18 +400,43 @@ describe('AppWrapper — save conflict handling', () => { expect(useMessages().showErrorMessage).toHaveBeenCalledTimes(1) }) - it('shows the conflict dialog when the session is not connected', async () => { + // `disconnected` and `connecting` are transient - the provider reconnects on + // its own - so the conflict must not take the room down with it. + it.each(['local', 'disconnected', 'connecting'] as const)( + 'shows the conflict dialog without giving up the room when the status is %s', + async (status) => { + const putFileContents = vi.fn().mockRejectedValue(conflict()) + const s = setup({ status, putFileContents }) + await nextTick() + await s.resolveResource(FILE_A) + await s.resolveContent('content of a') + await s.edit('edited content') + await s.pressCtrlS() + + expect(putFileContents).toHaveBeenCalledTimes(1) + expect(s.getFileContents).toHaveBeenCalledTimes(1) // the initial load only + expect(useMessages().showErrorMessage).toHaveBeenCalledTimes(1) + expect(s.markConflicted).not.toHaveBeenCalled() + } + ) + + // A failed refetch or a failed retry is just as likely a network blip as an + // external write, so the user gets the popup but keeps the session. + it('keeps the room when the reconciliation itself fails', async () => { const putFileContents = vi.fn().mockRejectedValue(conflict()) - const s = setup({ status: 'local', putFileContents }) + const s = setup({ status: 'connected', putFileContents }) await nextTick() await s.resolveResource(FILE_A) await s.resolveContent('content of a') await s.edit('edited content') - await s.pressCtrlS() - expect(putFileContents).toHaveBeenCalledTimes(1) - expect(s.getFileContents).toHaveBeenCalledTimes(1) // the initial load only + const pressed = s.pressCtrlS() + await flushPromises() + s.rejectContent(new Error('network down')) + await pressed + expect(useMessages().showErrorMessage).toHaveBeenCalledTimes(1) + expect(s.markConflicted).not.toHaveBeenCalled() }) // Regression: the retry ran for any conflict inside a connected session, so @@ -527,6 +572,76 @@ describe('AppWrapper — save conflict handling', () => { expect(s.beginSave).toHaveBeenCalled() }) + + // The 412 path already tells the user; the session has to give up the room + // too, or the toolbar keeps claiming everything is fine while the doc is + // detached from what is on disk. + it('marks the session conflicted when an external write caused the conflict', async () => { + const putFileContents = vi.fn().mockRejectedValue(conflict()) + const s = setup({ status: 'connected', writtenByRoom: false, putFileContents }) + await nextTick() + await s.resolveResource(FILE_A) + await s.resolveContent('content of a') + await s.edit('edited content') + + const pressed = s.pressCtrlS() + await flushPromises() + await s.resolveContent('content written by a desktop client') + await pressed + + // Silently: `saveFileTask` shows the popup itself, so a second toast from + // the session would only duplicate it. + expect(s.markConflicted).toHaveBeenCalledWith(false) + expect(useMessages().showErrorMessage).toHaveBeenCalledTimes(1) + }) + + // The other direction: the session noticed the external change first (a peer + // is re-seeding the room) and asks the wrapper to explain it. + it('shows the conflict message when the session reports an external change', async () => { + const s = setup({ status: 'connected' }) + await nextTick() + await s.resolveResource(FILE_A) + await s.resolveContent('content of a') + + s.session().onConflict() + await nextTick() + + const { showErrorMessage } = useMessages() + expect(showErrorMessage).toHaveBeenCalledTimes(1) + expect(vi.mocked(showErrorMessage).mock.calls[0][0].desc).toContain( + 'updated outside this window' + ) + }) + + it('keeps autosaving while the session is healthy', async () => { + vi.useFakeTimers({ shouldAdvanceTime: true }) + const s = setup({ status: 'connected' }) + await nextTick() + await s.resolveResource(FILE_A) + await s.resolveContent('content of a') + await s.edit('edited content') + + await vi.advanceTimersByTimeAsync(130_000) + + expect(s.putFileContents).toHaveBeenCalledTimes(1) + }) + + // Every attempt would fail the same way, so the popup would come back every + // interval until the user closes the tab. + it('skips the autosave while the session is conflicted', async () => { + vi.useFakeTimers({ shouldAdvanceTime: true }) + const s = setup({ status: 'connected' }) + await nextTick() + await s.resolveResource(FILE_A) + await s.resolveContent('content of a') + await s.edit('edited content') + + s.isConflicted.value = true + await nextTick() + await vi.advanceTimersByTimeAsync(130_000) + + expect(s.putFileContents).not.toHaveBeenCalled() + }) }) describe('AppWrapper — locked session', () => { diff --git a/packages/web-pkg/tests/unit/composables/yjs/useYjsSession.spec.ts b/packages/web-pkg/tests/unit/composables/yjs/useYjsSession.spec.ts index 236fbb7762d..ab31f30f531 100644 --- a/packages/web-pkg/tests/unit/composables/yjs/useYjsSession.spec.ts +++ b/packages/web-pkg/tests/unit/composables/yjs/useYjsSession.spec.ts @@ -127,6 +127,7 @@ function setupSession({ adapter = testAdapter as YjsAdapter, enabled = true, isReadOnly = false, + hasUnsavedChanges = false, // Mirrors what AppWrapper does with a reported etag: it writes it back into // the resource it passes in. Off by default so most tests can assert on the // callback alone, on for the ones about what that write-back does to the @@ -137,8 +138,10 @@ function setupSession({ const adapterRef = shallowRef(adapter) const enabledRef = ref(enabled) const isReadOnlyRef = ref(isReadOnly) + const hasUnsavedChangesRef = ref(hasUnsavedChanges) const contentRef = ref(currentContent) const onContentChange = vi.fn() + const onConflict = vi.fn() const onServerContentChange = vi.fn() const onEtagChange = vi.fn((etag: string) => { if (!mirrorEtagToResource) return @@ -157,7 +160,9 @@ function setupSession({ documentPrefix: 'test-app', onContentChange, onServerContentChange, - onEtagChange + onEtagChange, + hasUnsavedChanges: hasUnsavedChangesRef, + onConflict }) }, { @@ -178,10 +183,12 @@ function setupSession({ adapterRef, enabledRef, isReadOnlyRef, + hasUnsavedChangesRef, contentRef, onContentChange, onServerContentChange, onEtagChange, + onConflict, get session() { return session }, @@ -843,13 +850,15 @@ describe('useYjsSession — stale-state recovery', () => { ourEtag = 'etag-new', adapter = testAdapter as YjsAdapter, roomContent = 'stale room content', - isReadOnly = false + isReadOnly = false, + hasUnsavedChanges = false } = {}) { const s = setupSession({ yjsServerUrl, currentContent, adapter, isReadOnly, + hasUnsavedChanges, resource: makeResource({ etag: ourEtag }) }) await flushPromises() @@ -900,6 +909,21 @@ describe('useYjsSession — stale-state recovery', () => { expect(meta.get('etag')).toBe('etag-new') }) + // Regression: reset and hydrate ran as separate transactions, so a mounted + // editor observed an empty doc in between and padded it with an empty + // paragraph that survived the re-seed. + it('never exposes the emptied doc to observers while re-seeding', async () => { + const s = await syncIntoStaleRoom({ currentContent: 'fresh body' }) + const seen: string[] = [] + const text = s.ydoc!.getText(SHARED_TEXT_KEY) + text.observe(() => seen.push(text.toString())) + + vi.advanceTimersByTime(200) + await flushPromises() + + expect(seen).toEqual(['fresh body']) + }) + // Regression: the election was "lowest awareness clientId wins" over every // peer in the room, so a client that never saw the drift - and therefore // holds no fresh body - could win and publish its own older copy. @@ -1039,6 +1063,174 @@ describe('useYjsSession — stale-state recovery', () => { expect(meta.get('nativeEtag')).toBe('etag-new') expect(meta.get('recoveryClientId')).toBeUndefined() }) + + // A peer that holds unsaved work must not follow the reset: it arrives as + // plain CRDT updates and would wipe that work with no warning. Leaving the + // room keeps the local doc, and the caller surfaces its conflict message. + it('leaves the room instead of following a recovery that would drop unsaved work', async () => { + const s = setupSession({ + yjsServerUrl, + currentContent: 'my unsaved body', + resource: makeResource({ etag: 'etag-current' }), + hasUnsavedChanges: true + }) + await flushPromises() + providerInstances[0].triggerSynced() + vi.advanceTimersByTime(200) + await flushPromises() + + const doc = s.ydoc! + // A peer joined, found the file changed on disk and claimed the recovery. + const peer = new Y.Doc() + Y.applyUpdate(peer, Y.encodeStateAsUpdate(doc)) + peer.transact(() => { + const peerMeta = peer.getMap(META_KEY) + peerMeta.set('nativeEtag', 'etag-external') + peerMeta.set('recoveryClientId', peer.clientID) + peerMeta.set('isStale', true) + }) + Y.applyUpdate(doc, Y.encodeStateAsUpdate(peer, Y.encodeStateVector(doc))) + vi.advanceTimersByTime(200) + await flushPromises() + + expect(unref(s.session.isConflicted)).toBe(true) + expect(unref(s.session.status)).toBe('disconnected') + expect(s.onConflict).toHaveBeenCalledTimes(1) + expect(providerInstances[0].disconnect).toHaveBeenCalled() + // Detached too, or the doc's update handler keeps feeding the socket. + expect(providerInstances[0].detach).toHaveBeenCalled() + // The work is still on screen, and we did not run the recovery ourselves. + expect(doc.getText(SHARED_TEXT_KEY).toString()).toBe('my unsaved body') + expect(doc.getMap(META_KEY).get('isStale')).toBe(true) + }) + + // The flip side: nothing to lose, so the peer follows the recovery. + it('follows a peer recovery when there is no unsaved work', async () => { + const s = setupSession({ + yjsServerUrl, + currentContent: 'room body', + resource: makeResource({ etag: 'etag-current' }) + }) + await flushPromises() + providerInstances[0].triggerSynced() + vi.advanceTimersByTime(200) + await flushPromises() + + const doc = s.ydoc! + const peer = new Y.Doc() + Y.applyUpdate(peer, Y.encodeStateAsUpdate(doc)) + peer.transact(() => { + const peerMeta = peer.getMap(META_KEY) + peerMeta.set('nativeEtag', 'etag-external') + peerMeta.set('recoveryClientId', peer.clientID) + peerMeta.set('isStale', true) + }) + Y.applyUpdate(doc, Y.encodeStateAsUpdate(peer, Y.encodeStateVector(doc))) + vi.advanceTimersByTime(200) + await flushPromises() + + expect(unref(s.session.isConflicted)).toBe(false) + expect(unref(s.session.status)).not.toBe('disconnected') + expect(s.onConflict).not.toHaveBeenCalled() + }) + + // The recovering client detected the drift itself, so it holds the fresh + // body and there is nothing of its own to lose - dirty or not. + it('still recovers when it detected the drift itself', async () => { + const s = await syncIntoStaleRoom({ hasUnsavedChanges: true }) + vi.advanceTimersByTime(200) + await flushPromises() + + expect(unref(s.session.isConflicted)).toBe(false) + expect(s.onConflict).not.toHaveBeenCalled() + expect(s.ydoc!.getText(SHARED_TEXT_KEY).toString()).toBe('fresh body') + }) + + // Without the stamp, peers see the recovered body as a remote edit and go + // dirty over content that is demonstrably on disk - and autosave it back. + it('stamps the recovery commit as a save', async () => { + const s = await syncIntoStaleRoom() + vi.advanceTimersByTime(200) + await flushPromises() + + const meta = s.ydoc!.getMap(META_KEY) + expect(meta.get('savedStateVector')).toBeInstanceOf(Uint8Array) + expect(meta.get('lastSavedAt')).toBeTypeOf('number') + }) + + // The receiving end of that stamp: the recovered body becomes the peer's + // server content, so it is clean rather than holding a phantom edit. + it('takes a peer recovery as a save rather than an unsaved change', async () => { + const s = setupSession({ + yjsServerUrl, + currentContent: 'room body', + resource: makeResource({ etag: 'etag-old' }) + }) + await flushPromises() + providerInstances[0].triggerSynced() + vi.advanceTimersByTime(200) + await flushPromises() + + // A peer holding the fresh body runs the whole recovery: reset, re-seed, + // commit - exactly what `recoverFromStaleState` writes. + const doc = s.ydoc! + const peer = new Y.Doc() + Y.applyUpdate(peer, Y.encodeStateAsUpdate(doc)) + peer.transact(() => { + const text = peer.getText(SHARED_TEXT_KEY) + text.delete(0, text.length) + text.insert(0, 'recovered body') + const peerMeta = peer.getMap(META_KEY) + peerMeta.set('etag', 'etag-new') + peerMeta.set('savedStateVector', Y.encodeStateVector(peer)) + peerMeta.set('lastSavedAt', Date.now()) + }) + Y.applyUpdate(doc, Y.encodeStateAsUpdate(peer, Y.encodeStateVector(doc))) + await flushPromises() + + expect(unref(s.session.isConflicted)).toBe(false) + expect(s.onEtagChange).toHaveBeenCalledWith('etag-new') + expect(s.onServerContentChange).toHaveBeenCalledWith('recovered body') + }) + + it('gives up the room only once', async () => { + const s = setupSession({ yjsServerUrl, currentContent: 'x' }) + await flushPromises() + providerInstances[0].triggerSynced() + vi.advanceTimersByTime(200) + await flushPromises() + + s.session.markConflicted() + s.session.markConflicted() + + expect(s.onConflict).toHaveBeenCalledTimes(1) + expect(providerInstances[0].disconnect).toHaveBeenCalledOnce() + }) + + it('never conflicts in local mode', async () => { + const s = setupSession({ currentContent: 'x' }) + await flushPromises() + + s.session.markConflicted() + + expect(unref(s.session.isConflicted)).toBe(false) + expect(unref(s.session.status)).toBe('local') + expect(s.onConflict).not.toHaveBeenCalled() + }) + + it('clears the conflict when the session is rebuilt', async () => { + const s = setupSession({ yjsServerUrl, currentContent: 'x' }) + await flushPromises() + s.session.markConflicted() + expect(unref(s.session.isConflicted)).toBe(true) + + s.enabledRef.value = false + await flushPromises() + s.enabledRef.value = true + await flushPromises() + + expect(unref(s.session.isConflicted)).toBe(false) + }) }) describe('useYjsSession — etag mirror', () => { From c1be3c4a35b9741110c6c40735d4c2ad7fb3fa5b Mon Sep 17 00:00:00 2001 From: Jannik Stehle Date: Thu, 27 Aug 2026 14:07:20 +0200 Subject: [PATCH 2/4] feat(yjs): restore unsaved work dropped during a resync The isStale guard only holds when the flag arrives ahead of the rewrite it announces. A peer that was offline while a recovery ran gets flag, rewrite and commit back as one merged update, so the isStale guard never fires. As a result, unsaved local changes would just be overwritten with the new content. Key the guard on a new recoveryEpoch meta value that is bumped with every isStale and never cleared. If the flag is still up the doc is untouched and the peer just leaves the room. Otherwise the rewrite has already dropped the work, so leave the room and re-seed the doc from the last reported content. --- .../src/composables/yjs/useYjsSession.ts | 109 ++++++++++-- .../composables/yjs/useYjsSession.spec.ts | 166 ++++++++++++++++++ 2 files changed, 263 insertions(+), 12 deletions(-) diff --git a/packages/web-pkg/src/composables/yjs/useYjsSession.ts b/packages/web-pkg/src/composables/yjs/useYjsSession.ts index 73a96cc7bc5..8d14e292cbe 100644 --- a/packages/web-pkg/src/composables/yjs/useYjsSession.ts +++ b/packages/web-pkg/src/composables/yjs/useYjsSession.ts @@ -171,6 +171,11 @@ interface SessionMeta { nativeEtag: string /** The client elected to run the stale-state recovery. */ recoveryClientId: number + /** + * Bumped with every `isStale`. Unlike the flag it is never cleared, so a + * recovery that arrives whole in one merged update still shows as a change. + */ + recoveryEpoch: number /** Doc state behind the last written file, see `lastReportedStateVector`. */ savedStateVector: Uint8Array /** A writer announced it is seeding the room. */ @@ -304,6 +309,12 @@ export function useYjsSession(options: YjsSessionOptions): YjsSession { */ let lastReportedStateVector: Uint8Array | null = null + /** + * The content behind `lastReportedStateVector`. Kept so a resync that + * dropped our unsaved work can be undone, see `undoResyncWipe`. + */ + let lastReportedContent: string | null = null + /** * `lastReportedStateVector` frozen at the moment the caller began a save, * so a report landing while the PUT is in flight cannot move it. See @@ -383,6 +394,7 @@ export function useYjsSession(options: YjsSessionOptions): YjsSession { if (!unref(effectiveReadOnly)) claimRecovery(doc, meta) doc.transact(() => { if (nativeEtag) meta.set('nativeEtag', nativeEtag) + meta.set('recoveryEpoch', (meta.get('recoveryEpoch') ?? 0) + 1) meta.set('isStale', true) }) } @@ -698,6 +710,7 @@ export function useYjsSession(options: YjsSessionOptions): YjsSession { .then((value) => { if (value === null) return lastReportedStateVector = vectorAtSerialize + lastReportedContent = value onContentChange(value) }) .catch((e) => console.error('[yjs] serialize for content update failed:', e)) @@ -708,6 +721,7 @@ export function useYjsSession(options: YjsSessionOptions): YjsSession { } return { onDocUpdate, cancel } } + type ContentReporter = ReturnType /** Connects a Hocuspocus provider and arms the connect timeout. */ function connectRemote(doc: Y.Doc, name: string, serverUrl: string) { @@ -785,10 +799,67 @@ export function useYjsSession(options: YjsSessionOptions): YjsSession { } /** - * `_oc_meta` is the side channel for save/stale coordination. + * A recovery may have deleted our unsaved work if it ran while we were offline. + * Leave the room and re-seed the doc from the last content we reported. + */ + function undoResyncWipe(doc: Y.Doc, prov: HocuspocusProvider | null, reporter: ContentReporter) { + const content = lastReportedContent + const current = toValue(adapter) + + // The doc holds the rewritten body from here on, and it must never reach + // the caller as our own content - the next save would put it on disk. + isRewritingDoc = true + reporter.cancel() + + if (content === null || typeof current.reset !== 'function') { + // No conflict toast, there are no changes to be copied - they're gone. + markConflicted(false) + console.error('[yjs] the room discarded unsaved work that cannot be restored') + lockForReload( + prov, + $gettext( + 'This file was changed externally and your unsaved changes could not be restored. Please reload.' + ) + ) + return + } + + markConflicted() + + // Deferred: Yjs is still cleaning up the transaction this reacts to. + // `markConflicted` stopped the provider, so nothing else touches the doc + // in between. + queueMicrotask(() => { + if (doc.isDestroyed) return + try { + doc.transact(() => { + current.reset?.(doc) + current.hydrate(doc, content) + }, STALE_RECOVERY_RESET_ORIGIN) + isRewritingDoc = false + } catch (e) { + // `reset` already committed, so the doc may be empty. Keep the rewrite + // flag up and lock: nothing may report or autosave what is left. + console.error('[yjs] restoring unsaved work after a resync failed:', e) + lockForReload( + prov, + $gettext( + 'This file was changed externally and restoring your unsaved changes failed. Please reload.' + ) + ) + } + }) + } + + /** + * `_oc_meta` is the side channel for save/stale/version coordination. * Adapters bind to their own shared types and never see it. */ - function createMetaObserver(doc: Y.Doc, prov: HocuspocusProvider | null) { + function createMetaObserver( + doc: Y.Doc, + prov: HocuspocusProvider | null, + reporter: ContentReporter + ) { const meta = sessionMeta(doc) return function metaObserver(event: Y.YMapEvent, transaction: Y.Transaction) { // The initial sync replays the room's whole meta map through this @@ -798,13 +869,19 @@ export function useYjsSession(options: YjsSessionOptions): YjsSession { // Remote ops carry no string origin, so this means "a peer is acting // right now" - not the initial sync, not one of our own writes. const isRemoteMetaWrite = isMidSession && !OWN_META_ORIGINS.includes(transaction.origin) + // Handled further down, but needed here to keep the fan-out out of it. + const resyncWouldDropOurWork = + event.keysChanged.has('recoveryEpoch') && isRemoteMetaWrite && toValue(hasUnsavedChanges) // Peer-save fan-out. The fresh etag keeps our next If-Match correct. // The content only follows when the peer's snapshot covers everything // we hold; otherwise our dirty flag would drop over edits that never // reached the peer's PUT and they could leave with the tab. Re-checked // after serializing because a keystroke can land while that runs. - if (isRemoteMetaWrite) { + // Not on a recovery that costs us our work: the rewriting peer's etag + // would make our next If-Match match, so a manual save would overwrite + // the body they just put on disk. + if (isRemoteMetaWrite && !resyncWouldDropOurWork) { if (event.keysChanged.has('etag')) { const newEtag = meta.get('etag') if (newEtag) onEtagChange(newEtag) @@ -835,17 +912,22 @@ export function useYjsSession(options: YjsSessionOptions): YjsSession { return } + // A peer wipes and re-seeds the room. Following that would silently + // drop our unsaved edits, so leave the room and keep our own doc. + if (resyncWouldDropOurWork) { + if (meta.get('isStale') === true) { + // The flag came ahead of the rewrite, our doc is untouched. + markConflicted() + } else { + // Recovery already ran, our unsaved edits are gone. Attempt to restore them. + undoResyncWipe(doc, prov, reporter) + } + return + } + // Stale-state signal: every peer sees it, only the claimed one gets // past the guards in `recoverFromStaleState`. if (event.keysChanged.has('isStale') && meta.get('isStale') === true) { - // A peer is about to wipe and re-seed the room. Following that would - // silently drop our unsaved edits, so leave the room and keep the - // local doc instead. Our own `flagStale` (raised while `isReady` is - // still false) and the initial sync replay stay on the recovery path. - if (isRemoteMetaWrite && toValue(hasUnsavedChanges)) { - markConflicted() - return - } void recoverFromStaleState(doc, prov) } } @@ -867,8 +949,11 @@ export function useYjsSession(options: YjsSessionOptions): YjsSession { isConflicted.value = false isReady.value = false hasLocalOnlyContent = false + // `undoResyncWipe` leaves this up for good when it cannot restore. + isRewritingDoc = false staleRecoveryContent = null lastReportedStateVector = null + lastReportedContent = null pendingSaveStateVector = null if (!key) { @@ -901,7 +986,7 @@ export function useYjsSession(options: YjsSessionOptions): YjsSession { } const meta = sessionMeta(doc) - const metaObserver = createMetaObserver(doc, prov) + const metaObserver = createMetaObserver(doc, prov, reporter) meta.map.observe(metaObserver) ydoc.value = doc diff --git a/packages/web-pkg/tests/unit/composables/yjs/useYjsSession.spec.ts b/packages/web-pkg/tests/unit/composables/yjs/useYjsSession.spec.ts index ab31f30f531..9cdd77b3407 100644 --- a/packages/web-pkg/tests/unit/composables/yjs/useYjsSession.spec.ts +++ b/packages/web-pkg/tests/unit/composables/yjs/useYjsSession.spec.ts @@ -36,6 +36,7 @@ interface MockProvider { detach: ReturnType setAwarenessField: ReturnType triggerSynced(): void + triggerStatus(status: string): void triggerAuthFailed(reason: string): void } @@ -72,6 +73,9 @@ vi.mock('@hocuspocus/provider', async () => { triggerSynced() { this._opts.onSynced?.({ state: true }) } + triggerStatus(status: string) { + this._opts.onStatus?.({ status }) + } triggerAuthFailed(reason: string) { this._opts.onAuthenticationFailed?.({ reason }) } @@ -1087,6 +1091,7 @@ describe('useYjsSession — stale-state recovery', () => { const peerMeta = peer.getMap(META_KEY) peerMeta.set('nativeEtag', 'etag-external') peerMeta.set('recoveryClientId', peer.clientID) + peerMeta.set('recoveryEpoch', 1) peerMeta.set('isStale', true) }) Y.applyUpdate(doc, Y.encodeStateAsUpdate(peer, Y.encodeStateVector(doc))) @@ -1123,6 +1128,7 @@ describe('useYjsSession — stale-state recovery', () => { const peerMeta = peer.getMap(META_KEY) peerMeta.set('nativeEtag', 'etag-external') peerMeta.set('recoveryClientId', peer.clientID) + peerMeta.set('recoveryEpoch', 1) peerMeta.set('isStale', true) }) Y.applyUpdate(doc, Y.encodeStateAsUpdate(peer, Y.encodeStateVector(doc))) @@ -1218,6 +1224,166 @@ describe('useYjsSession — stale-state recovery', () => { expect(s.onConflict).not.toHaveBeenCalled() }) + /** + * A session that holds unsaved work and has typed since its last report, + * plus a peer doc that already carries every one of its ops - the state the + * room needs in order to be able to delete them. + */ + async function syncedPeerHoldingOurOps(adapter: YjsAdapter = testAdapter) { + vi.useFakeTimers({ shouldAdvanceTime: true }) + const s = setupSession({ + yjsServerUrl, + currentContent: 'seed', + hasUnsavedChanges: true, + adapter + }) + await flushPromises() + providerInstances[0].triggerSynced() + vi.advanceTimersByTime(200) + await flushPromises() + + s.ydoc!.getText(SHARED_TEXT_KEY).insert(4, ' ours') + vi.advanceTimersByTime(400) + await flushPromises() + expect(s.onContentChange).toHaveBeenLastCalledWith('seed ours') + + const peer = new Y.Doc() + Y.applyUpdate(peer, Y.encodeStateAsUpdate(s.ydoc!)) + return { s, peer } + } + + /** + * The room rewriting itself the way a recovery does, on a peer doc: flag, + * rewrite and commit, all of it while we are not looking. + */ + function rewriteRoom(peer: Y.Doc, body: string) { + const peerMeta = peer.getMap(META_KEY) + peer.transact(() => { + peerMeta.set('recoveryEpoch', 1) + peerMeta.set('isStale', true) + }) + peer.transact(() => { + const text = peer.getText(SHARED_TEXT_KEY) + text.delete(0, text.length) + text.insert(0, body) + }) + peer.transact(() => { + peerMeta.delete('isStale') + peerMeta.set('etag', 'etag-external') + peerMeta.set('savedStateVector', Y.encodeStateVector(peer)) + peerMeta.set('lastSavedAt', 1) + }) + } + + /** + * A reconnect: everything the peer did since we last saw it arrives as one + * merged update, then the provider reports the sync. + */ + async function resyncFromPeer(s: ReturnType, peer: Y.Doc) { + Y.applyUpdate(s.ydoc!, Y.encodeStateAsUpdate(peer, Y.encodeStateVector(s.ydoc!))) + providerInstances[0].triggerSynced() + await flushPromises() + } + + // Regression: a client that was offline while a recovery ran gets the whole + // thing back as one merged update, which Yjs integrates before any observer + // fires - and which raises and clears `isStale` in one go, so an observer + // keyed on the flag sees no net change and never reacted at all. The unsaved + // work was deleted silently and the next autosave wrote over it. + it('restores unsaved work the room dropped while we were away', async () => { + const { s, peer } = await syncedPeerHoldingOurOps() + providerInstances[0].triggerStatus('disconnected') + + rewriteRoom(peer, 'body from a desktop client') + await resyncFromPeer(s, peer) + + expect(s.onConflict).toHaveBeenCalledTimes(1) + expect(unref(s.session.isConflicted)).toBe(true) + expect(providerInstances[0].disconnect).toHaveBeenCalled() + // The work is back in the doc, so the editor still shows it and the user + // can copy it out or "Save As". + expect(s.ydoc!.getText(SHARED_TEXT_KEY).toString()).toBe('seed ours') + }) + + // The debounce window outlives the restore, so what it eventually reports + // must be the restored work and never the body the room rewrote itself to. + it('never reports the rewritten body as our own content', async () => { + const { s, peer } = await syncedPeerHoldingOurOps() + providerInstances[0].triggerStatus('disconnected') + + rewriteRoom(peer, 'body from a desktop client') + await resyncFromPeer(s, peer) + vi.advanceTimersByTime(400) + await flushPromises() + + expect(s.onContentChange).not.toHaveBeenCalledWith('body from a desktop client') + expect(s.onContentChange).toHaveBeenLastCalledWith('seed ours') + }) + + // The rewriting peer's etag would make our next If-Match match what is on + // disk, so a manual save out of the conflict would overwrite their body. + it('does not adopt the etag of the peer that rewrote the room', async () => { + const { s, peer } = await syncedPeerHoldingOurOps() + providerInstances[0].triggerStatus('disconnected') + + rewriteRoom(peer, 'body from a desktop client') + await resyncFromPeer(s, peer) + await flushPromises() + + expect(s.onEtagChange).not.toHaveBeenCalledWith('etag-external') + expect(s.onServerContentChange).not.toHaveBeenCalled() + }) + + // The work is already gone at this point, so a conflict message telling the + // user to copy their changes out would be a lie. Lock instead. + // + // Also the one path where the re-armed report is not masked by a restore: + // type observers run before the doc `update` event, so the merged update + // arms a fresh debounce right after we react to it. Cancelling the pending + // report alone left the rewritten body reaching the caller as our own. + it('locks when the dropped work cannot be restored', async () => { + silenceConsoleError() + const { reset: _reset, ...resetlessAdapter } = testAdapter + const { s, peer } = await syncedPeerHoldingOurOps(resetlessAdapter) + providerInstances[0].triggerStatus('disconnected') + + rewriteRoom(peer, 'body from a desktop client') + await resyncFromPeer(s, peer) + vi.advanceTimersByTime(400) + await flushPromises() + + expect(unref(s.session.isLockedForReload)).toBe(true) + expect(unref(s.session.error)).toBeInstanceOf(Error) + expect(s.onConflict).not.toHaveBeenCalled() + expect(s.onContentChange).not.toHaveBeenCalledWith('body from a desktop client') + }) + + // Nothing of ours was dropped, so following the room loses nothing. + // Restoring here would throw the peer's edits away instead. + it('follows the room when a resync did not drop our ops', async () => { + const { s, peer } = await syncedPeerHoldingOurOps() + providerInstances[0].triggerStatus('disconnected') + + peer.transact(() => peer.getText(SHARED_TEXT_KEY).insert(0, 'theirs ')) + await resyncFromPeer(s, peer) + + expect(s.onConflict).not.toHaveBeenCalled() + expect(s.ydoc!.getText(SHARED_TEXT_KEY).toString()).toBe('theirs seed ours') + }) + + // A peer removing text we typed is ordinary collaboration, not a recovery. + it('leaves the room alone when a peer deletes text we typed mid-session', async () => { + const { s, peer } = await syncedPeerHoldingOurOps() + + peer.transact(() => peer.getText(SHARED_TEXT_KEY).delete(4, 5)) + Y.applyUpdate(s.ydoc!, Y.encodeStateAsUpdate(peer, Y.encodeStateVector(s.ydoc!))) + await flushPromises() + + expect(s.onConflict).not.toHaveBeenCalled() + expect(unref(s.session.isConflicted)).toBe(false) + expect(s.ydoc!.getText(SHARED_TEXT_KEY).toString()).toBe('seed') + }) + it('clears the conflict when the session is rebuilt', async () => { const s = setupSession({ yjsServerUrl, currentContent: 'x' }) await flushPromises() From 8b44f961820227b7a02808bc543c3ea326f7bc55 Mon Sep 17 00:00:00 2001 From: Jannik Stehle Date: Thu, 27 Aug 2026 15:46:03 +0200 Subject: [PATCH 3/4] test(e2e): add yjs status checks --- .../editor/components/TextEditorToolbar.vue | 3 ++ tests/e2e/features/shares/link.feature | 4 +- .../features/yjs/collaborativeEditing.feature | 14 +++++++ tests/e2e/features/yjs/conflicts.feature | 39 +++++++++++++++++-- tests/e2e/features/yjs/publicLinks.feature | 2 +- tests/e2e/steps/ui/links.ts | 2 +- tests/e2e/steps/ui/resources.ts | 30 ++++++++++++++ .../support/objects/app-files/utils/editor.ts | 11 +++++- 8 files changed, 96 insertions(+), 9 deletions(-) diff --git a/packages/web-pkg/src/editor/components/TextEditorToolbar.vue b/packages/web-pkg/src/editor/components/TextEditorToolbar.vue index 2b380742eaf..56f786b84bd 100644 --- a/packages/web-pkg/src/editor/components/TextEditorToolbar.vue +++ b/packages/web-pkg/src/editor/components/TextEditorToolbar.vue @@ -154,6 +154,7 @@ v-oc-tooltip="collaborationStatusLabel" class="text-editor-toolbar-collaboration-status ml-2 inline-flex shrink-0 items-center" :aria-label="collaborationStatusLabel" + :data-test-yjs-status="yjsStatus" > { return !!unref(textEditor.editor) }) +const yjsStatus = computed(() => unref(textEditor.yjsStatus)) + const showCollaborationStatusIndicator = computed(() => { const status = unref(textEditor.yjsStatus) return status !== YjsStatus.Local && status !== null diff --git a/tests/e2e/features/shares/link.feature b/tests/e2e/features/shares/link.feature index ced2dc76736..d2342a242ae 100644 --- a/tests/e2e/features/shares/link.feature +++ b/tests/e2e/features/shares/link.feature @@ -266,13 +266,13 @@ Feature: link | resource | password | | lorem.txt | %public% | When "Alice" tries to sets a new password "OpenCloud-1" of the public link named "Unnamed link" of resource "lorem.txt" - Then "Alice" should see an error message + Then "Alice" should see a password error message """ Unfortunately, your password is commonly used. please pick a harder-to-guess password for your safety """ And "Alice" closes the public link password dialog box When "Alice" tries to sets a new password "OpenCloud-1" of the public link named "Unnamed link" of resource "lorem.txt" - Then "Alice" should see an error message + Then "Alice" should see a password error message """ Unfortunately, your password is commonly used. please pick a harder-to-guess password for your safety """ diff --git a/tests/e2e/features/yjs/collaborativeEditing.feature b/tests/e2e/features/yjs/collaborativeEditing.feature index 3a66e491256..0d64ee2bb24 100644 --- a/tests/e2e/features/yjs/collaborativeEditing.feature +++ b/tests/e2e/features/yjs/collaborativeEditing.feature @@ -20,12 +20,16 @@ Feature: yjs collaborative editing And "Brian" logs in And "Brian" navigates to the shared with me page And "Brian" opens file "example.md" via "text-editor" using the context menu + And "Brian" should see the following yjs status + | status | + | Connected | And "Brian" is in a text-editor And "Brian" enters the text "Brian says hello" in editor "TextEditor" And "Carol" logs in And "Carol" navigates to the shared with me page And "Carol" opens file "example.md" via "text-editor" using the context menu + And "Carol" should not see a yjs status And "Carol" is in a text-editor And "Carol" should see the text "Brian says hello" in the text-editor And "Carol" should not be able to edit the current file @@ -33,6 +37,9 @@ Feature: yjs collaborative editing And "Alice" logs in And "Alice" opens the "files" app And "Alice" opens file "example.md" via "text-editor" using the context menu + And "Alice" should see the following yjs status + | status | + | Connected | And "Alice" is in a text-editor Then "Alice" should see the text "Brian says hello" in the text-editor @@ -77,12 +84,16 @@ Feature: yjs collaborative editing And "Brian" logs in And "Brian" navigates to the project space "team" And "Brian" opens file "example.md" via "text-editor" using the context menu + And "Brian" should see the following yjs status + | status | + | Connected | And "Brian" is in a text-editor And "Brian" enters the text "Brian says hello" in editor "TextEditor" And "Carol" logs in And "Carol" navigates to the project space "team" And "Carol" opens file "example.md" via "text-editor" using the context menu + And "Carol" should not see a yjs status And "Carol" is in a text-editor And "Carol" should see the text "Brian says hello" in the text-editor And "Carol" should not be able to edit the current file @@ -91,6 +102,9 @@ Feature: yjs collaborative editing | Brian | And "Alice" opens file "example.md" via "text-editor" using the context menu + And "Alice" should see the following yjs status + | status | + | Connected | And "Alice" is in a text-editor Then "Alice" should see the text "Brian says hello" in the text-editor And "Alice" should see the collaboration carets of the following users diff --git a/tests/e2e/features/yjs/conflicts.feature b/tests/e2e/features/yjs/conflicts.feature index cef1956ddea..f2a089bda7f 100644 --- a/tests/e2e/features/yjs/conflicts.feature +++ b/tests/e2e/features/yjs/conflicts.feature @@ -23,6 +23,9 @@ Feature: yjs conflict handling And "Alice" logs in And "Alice" opens the "files" app And "Alice" opens file "textfile.ocnote" via "text-editor" using the context menu + And "Alice" should see the following yjs status + | status | + | Connected | And "Alice" enters the text "Alice says hello" in editor "TextEditor" # external update @@ -33,8 +36,14 @@ Feature: yjs conflict handling And "Alice" saves the file viewer expecting conflict error And "Alice" sees the current file as dirty + And "Alice" should see the following yjs status + | status | + | Disconnected | And "Alice" reloads the page Then "Alice" should see the text "some random text" in the text-editor + And "Alice" should see the following yjs status + | status | + | Connected | And "Alice" logs out Scenario: editor sees external file update when peer with edit permissions joins @@ -49,6 +58,9 @@ Feature: yjs conflict handling | user | role | kind | | Brian | Can edit | user | And "Alice" opens file "textfile.ocnote" via "text-editor" using the context menu + And "Alice" should see the following yjs status + | status | + | Connected | And "Alice" enters the text "Alice says hello" in editor "TextEditor" And "Alice" sees the current file as dirty And "Alice" saves the file viewer @@ -62,8 +74,14 @@ Feature: yjs conflict handling When "Brian" logs in And "Brian" navigates to the project space "team" And "Brian" opens file "textfile.ocnote" via "text-editor" using the context menu + And "Brian" should see the following yjs status + | status | + | Connected | Then "Brian" should see the text "some random text" in the text-editor And "Alice" should see the text "some random text" in the text-editor + And "Alice" should see the following yjs status + | status | + | Connected | And "Alice" logs out And "Brian" logs out @@ -79,6 +97,9 @@ Feature: yjs conflict handling | user | role | kind | | Brian | Can view | user | And "Alice" opens file "textfile.ocnote" via "text-editor" using the context menu + And "Alice" should see the following yjs status + | status | + | Connected | And "Alice" enters the text "Alice says hello" in editor "TextEditor" And "Alice" sees the current file as dirty And "Alice" saves the file viewer @@ -112,6 +133,9 @@ Feature: yjs conflict handling | user | role | kind | | Brian | Can edit | user | And "Alice" opens file "textfile.ocnote" via "text-editor" using the context menu + And "Alice" should see the following yjs status + | status | + | Connected | And "Alice" enters the text "Alice says hello" in editor "TextEditor" And "Alice" sees the current file as dirty And "Alice" saves the file viewer @@ -127,10 +151,17 @@ Feature: yjs conflict handling And "Brian" navigates to the project space "team" And "Brian" opens file "textfile.ocnote" via "text-editor" using the context menu Then "Brian" should see the text "some random text" in the text-editor - # FIXME: uncomment when https://github.com/opencloud-eu/web/issues/3103 is resolved - #And "Alice" should see the text "Alice update #2" in the text-editor - #And "Alice" should see the conflict dialog - And "Alice" should see the text "some random text" in the text-editor + And "Brian" should see the following yjs status + | status | + | Connected | + And "Alice" should see the text "Alice update #2" in the text-editor + And "Alice" should see an error message + """ + This file was updated outside this window. Please copy your changes or save the file under a new name (»Save As...«). + """ + And "Alice" should see the following yjs status + | status | + | Disconnected | And "Alice" logs out And "Brian" logs out \ No newline at end of file diff --git a/tests/e2e/features/yjs/publicLinks.feature b/tests/e2e/features/yjs/publicLinks.feature index a79efe81209..21b5e6ccfc9 100644 --- a/tests/e2e/features/yjs/publicLinks.feature +++ b/tests/e2e/features/yjs/publicLinks.feature @@ -55,9 +55,9 @@ Feature: yjs public editing When "Alice" opens file "example.md" via "text-editor" using the context menu And "Alice" should see the text "Anonymous says hello" in the text-editor And "Alice" enters the text "Alice says hello" in editor "TextEditor" + And "Alice" sees the current file as dirty And "Anonymous" should see the text "Anonymous says hello" in the text-editor And "Alice" saves the file viewer - And "Alice" sees the current file as dirty Then "Anonymous" should see the text "Anonymous says hello" in the text-editor When "Anonymous" reloads the page diff --git a/tests/e2e/steps/ui/links.ts b/tests/e2e/steps/ui/links.ts index faa778cd909..4d3246fd5a2 100644 --- a/tests/e2e/steps/ui/links.ts +++ b/tests/e2e/steps/ui/links.ts @@ -195,7 +195,7 @@ Then( ) Then( - '{string} should see an error message', + '{string} should see a password error message', async ({ world }: { world: World }, stepUser: any, errorMessage: string): Promise => { const { page } = world.actorsEnvironment.getActor({ key: stepUser }) const linkObject = new objects.applicationFiles.Link({ page }) diff --git a/tests/e2e/steps/ui/resources.ts b/tests/e2e/steps/ui/resources.ts index 1510ac3e8d4..2bf874dc4a6 100644 --- a/tests/e2e/steps/ui/resources.ts +++ b/tests/e2e/steps/ui/resources.ts @@ -1457,6 +1457,36 @@ Then( } ) +Then( + '{string} should see an error message', + async ({ world }: { world: World }, stepUser: string, errorMessage: string): Promise => { + const { page } = world.actorsEnvironment.getActor({ key: stepUser }) + await expect(editor.errorNotificationLocator(page, errorMessage)).toBeVisible() + } +) + +Then( + '{string} should see the following yjs status', + async ({ world }: { world: World }, stepUser: string, stepTable: DataTable): Promise => { + const { page } = world.actorsEnvironment.getActor({ key: stepUser }) + + for (const { status } of stepTable.hashes()) { + await expect(editor.yjsStatusLocator(page, status.toLowerCase())).toBeVisible() + } + } +) + +Then( + '{string} should not see a yjs status', + async ({ world }: { world: World }, stepUser: string): Promise => { + const { page } = world.actorsEnvironment.getActor({ key: stepUser }) + + // the editor must be loaded, otherwise the assertion passes trivially + await expect(editor.textEditorContentLocator(page)).toBeVisible() + await expect(editor.yjsStatusIndicatorLocator(page)).not.toBeVisible() + } +) + Then( '{string} should not be able to edit the current file', async ({ world }: { world: World }, stepUser: string): Promise => { diff --git a/tests/e2e/support/objects/app-files/utils/editor.ts b/tests/e2e/support/objects/app-files/utils/editor.ts index c5b009ef30c..997ec0b50c2 100644 --- a/tests/e2e/support/objects/app-files/utils/editor.ts +++ b/tests/e2e/support/objects/app-files/utils/editor.ts @@ -10,6 +10,7 @@ const collaborationCursorLabel = '.collaboration-cursor__label' const saveConflictDialog = '.oc-modal' const filesListUrl = /.*\/files\/(spaces|shares|link|search)\/.*/ const errorNotification = '.oc-notification-message-danger' +const yjsStatusIndicator = '.text-editor-toolbar-collaboration-status' const saveConflictDialogButtons: Record = { Save: '.oc-modal-body-actions-confirm', "Don't Save": '.oc-modal-body-actions-secondary', @@ -60,7 +61,15 @@ export const saveButtonLocator = (page: Page): Locator => page.locator(saveTextE export const textEditorContentLocator = (page: Page): Locator => page.locator(textEditorContent) -export const errorNotificationLocator = (page: Page): Locator => page.locator(errorNotification) +export const errorNotificationLocator = (page: Page, message?: string): Locator => + message + ? page.locator(errorNotification, { hasText: message }).first() + : page.locator(errorNotification).first() + +export const yjsStatusIndicatorLocator = (page: Page): Locator => page.locator(yjsStatusIndicator) + +export const yjsStatusLocator = (page: Page, status: string): Locator => + page.locator(`${yjsStatusIndicator}[data-test-yjs-status="${status}"]`) export const collaborationCaretLocator = (page: Page, displayName: string): Locator => page.locator(collaborationCursorLabel, { hasText: displayName }) From 6a1460e00652594aed3fa0d8031d9913006199be Mon Sep 17 00:00:00 2001 From: Jannik Stehle Date: Fri, 28 Aug 2026 14:22:20 +0200 Subject: [PATCH 4/4] feat: improve file conflict message wording --- packages/web-pkg/src/components/AppTemplates/AppWrapper.vue | 2 +- tests/e2e/features/yjs/conflicts.feature | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/web-pkg/src/components/AppTemplates/AppWrapper.vue b/packages/web-pkg/src/components/AppTemplates/AppWrapper.vue index 2d1fb2bd646..96e2cee11dd 100644 --- a/packages/web-pkg/src/components/AppTemplates/AppWrapper.vue +++ b/packages/web-pkg/src/components/AppTemplates/AppWrapper.vue @@ -518,7 +518,7 @@ function showExternalUpdateConflict(response: Response = null) { errorPopup( new HttpError( $gettext( - 'This file was updated outside this window. Please copy your changes or save the file under a new name (»Save As...«).' + 'This file was updated outside this window. Please copy your changes, save the file under a new name (»Save As...«) or reload the page to discard your changes.' ), response ) diff --git a/tests/e2e/features/yjs/conflicts.feature b/tests/e2e/features/yjs/conflicts.feature index f2a089bda7f..8fd1c4221e7 100644 --- a/tests/e2e/features/yjs/conflicts.feature +++ b/tests/e2e/features/yjs/conflicts.feature @@ -157,7 +157,7 @@ Feature: yjs conflict handling And "Alice" should see the text "Alice update #2" in the text-editor And "Alice" should see an error message """ - This file was updated outside this window. Please copy your changes or save the file under a new name (»Save As...«). + This file was updated outside this window. Please copy your changes, save the file under a new name (»Save As...«) or reload the page to discard your changes. """ And "Alice" should see the following yjs status | status |