From 869cc2d9f9f7a5ec7e69727acf1c5144107717dc Mon Sep 17 00:00:00 2001 From: Jonas Date: Tue, 3 Nov 2026 17:28:04 +0100 Subject: [PATCH 1/3] fix(sync): reset outdated document when the file changed outside of Nextcloud Writes that bypass Nextcloud, e.g. directly on external storage, do not trigger the NodeWritten listeners, so a stored document and its yjs state were reused when the file was opened again, and the editor showed outdated content. When opening a session, compare the stored checksum with the current file. If it differs and the document has no unsaved changes, reset the document so the client loads the current file content. Documents with unsaved changes are kept so the conflict is raised on the next save. Refs: #9236 Signed-off-by: Jonas Assisted-by: ClaudeCode:claude-opus-5-5 --- lib/Service/DocumentService.php | 13 ++++++-- tests/unit/Service/DocumentServiceTest.php | 39 ++++++++++++++++++++++ 2 files changed, 50 insertions(+), 2 deletions(-) diff --git a/lib/Service/DocumentService.php b/lib/Service/DocumentService.php index 969d6bcd6f0..fe37d17bdd4 100644 --- a/lib/Service/DocumentService.php +++ b/lib/Service/DocumentService.php @@ -102,8 +102,17 @@ public function isSaveFromText(): bool { public function getOrCreateDocument(Document $document): Document { $loaded = $this->documentMapper->load($document->getContextType(), $document->getContextId()); if ($loaded !== null) { - $this->logger->info('Keep previous document of ' . $document->toString()); - return $loaded; + if ($loaded->getChecksum() === $document->getChecksum() || $this->hasUnsavedChanges($loaded)) { + $this->logger->info('Keep previous document of ' . $document->toString()); + return $loaded; + } + // The file was changed without passing through Nextcloud, e.g. directly on external storage. + $this->logger->info('Reset outdated document of ' . $document->toString()); + try { + $this->resetDocument($document->getContextType(), $document->getContextId()); + } catch (DocumentHasUnsavedChangesException) { + return $loaded; + } } if (!$this->ensureDocumentsFolder()) { diff --git a/tests/unit/Service/DocumentServiceTest.php b/tests/unit/Service/DocumentServiceTest.php index 64087365f0f..cf1b42c8c9c 100644 --- a/tests/unit/Service/DocumentServiceTest.php +++ b/tests/unit/Service/DocumentServiceTest.php @@ -122,6 +122,14 @@ public function testCannotCheckUnsavedChangesWithoutDocumentId(): void { $this->documentService->hasUnsavedChanges(new Document()); } + private function createBuiltDocument(string $content): Document { + $document = new Document(); + $document->setContextType('file'); + $document->setContextId(42); + $document->setChecksum(DocumentService::computeCheckSum($content)); + return $document; + } + public function testNoAutosavingWhileSaveIsUnderWay(): void { $document = $this->createDocument('etag1', 1000, 'new content'); $context = $this->createMock(FileContext::class); @@ -134,4 +142,35 @@ public function testNoAutosavingWhileSaveIsUnderWay(): void { self::assertSame($result, $document); } + public function testKeepsDocumentWhenFileIsUnchanged(): void { + $loaded = $this->createDocument('etag1', 1000, 'content'); + $this->documentMapper->method('load')->willReturn($loaded); + $this->documentMapper->expects(self::never())->method('delete'); + $this->documentMapper->expects(self::never())->method('insert'); + + self::assertSame($loaded, $this->documentService->getOrCreateDocument($this->createBuiltDocument('content'))); + } + + public function testKeepsDocumentWithUnsavedChangesWhenFileChangedOutside(): void { + $loaded = $this->createDocument('etag1', 1000, 'content'); + $loaded->setLastSavedVersion(3); + $this->documentMapper->method('load')->willReturn($loaded); + $this->stepMapper->method('getLatestVersion')->willReturn(4); + $this->documentMapper->expects(self::never())->method('delete'); + $this->documentMapper->expects(self::never())->method('insert'); + + self::assertSame($loaded, $this->documentService->getOrCreateDocument($this->createBuiltDocument('changed outside'))); + } + + public function testResetsDocumentWhenFileChangedOutside(): void { + $loaded = $this->createDocument('etag1', 1000, 'content'); + $loaded->setLastSavedVersion(3); + $this->documentMapper->method('load')->willReturn($loaded); + $this->stepMapper->method('getLatestVersion')->willReturn(3); + $this->documentMapper->expects(self::once())->method('delete')->with($loaded); + $this->documentMapper->expects(self::once())->method('insert')->willReturnArgument(0); + + $built = $this->createBuiltDocument('changed outside'); + self::assertSame($built, $this->documentService->getOrCreateDocument($built)); + } } From b80b1c03d83a8ed314b025f170657232231de60b Mon Sep 17 00:00:00 2001 From: Jonas Date: Tue, 3 Nov 2026 17:28:53 +0100 Subject: [PATCH 2/3] fix(sync): show conflict dialog when saving fails with 409 Since conflicts are no longer reported by the sync endpoint, a save rejected because the file changed outside of Text returned 409 with the outside content, but SaveService ignored it. The editor kept retrying silently and the changes were never saved. Emit a SAVE_COLLISION error so the conflict resolution dialog is shown. Refs: #9236 Signed-off-by: Jonas Assisted-by: ClaudeCode:claude-opus-5-5 --- src/services/SaveService.ts | 9 ++++- src/tests/services/SaveService.spec.ts | 47 ++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 1 deletion(-) create mode 100644 src/tests/services/SaveService.spec.ts diff --git a/src/services/SaveService.ts b/src/services/SaveService.ts index d6a9ea436de..95a87d922e5 100644 --- a/src/services/SaveService.ts +++ b/src/services/SaveService.ts @@ -87,12 +87,19 @@ class SaveService { } catch (e) { logger.error('Failed to save document.', { error: e }) const response = ( - e as { response?: { status?: number, data?: { error?: string } } } + e as { response?: { status?: number, data?: { error?: string, outsideChange?: string } } } ).response if (response?.status === 403) { // Document is now read-only; permissionChange from sync will update the UI return } + if (response?.status === 409) { + this.bus.emit('error', { + type: ERROR_TYPE.SAVE_COLLISION, + data: { outsideChange: response.data?.outsideChange }, + }) + return + } if (response?.status === 412) { this.bus.emit('error', { type: ERROR_TYPE.LOAD_ERROR, diff --git a/src/tests/services/SaveService.spec.ts b/src/tests/services/SaveService.spec.ts new file mode 100644 index 00000000000..6f77eb0b9ba --- /dev/null +++ b/src/tests/services/SaveService.spec.ts @@ -0,0 +1,47 @@ +/** + * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: AGPL-3.0-or-later + */ + +import type { Connection } from '../../types/Connection.ts' + +import { describe, expect, it, vi } from 'vitest' +import { shallowRef } from 'vue' +import * as saveApi from '../../apis/save.ts' +import { SaveService } from '../../services/SaveService.ts' +import { ERROR_TYPE } from '../../types/ErrorType.ts' + +vi.mock('../../apis/save.ts') + +const connection = shallowRef({ + documentId: '123', + sessionId: 345, + sessionToken: 'sessionToken', + filePath: './', + baseVersionEtag: 'etag', +}) +function getSaveData() { + return { + version: 1, + autosaveContent: 'content', + documentState: 'state', + } +} + +describe('SaveService', () => { + it('emits a save collision when the file was changed outside', async () => { + vi.mocked(saveApi.save).mockRejectedValue({ + response: { status: 409, data: { outsideChange: 'outside content' } }, + }) + const saveService = new SaveService({ connection, getSaveData }) + const onError = vi.fn() + saveService.bus.on('error', onError) + + await saveService.save() + + expect(onError).toHaveBeenCalledWith({ + type: ERROR_TYPE.SAVE_COLLISION, + data: { outsideChange: 'outside content' }, + }) + }) +}) From 977bd1dfb216fe84892850997ad06f265f71c560 Mon Sep 17 00:00:00 2001 From: Jonas Date: Tue, 3 Nov 2026 18:21:19 +0100 Subject: [PATCH 3/3] test(playwright): test conflict resolution on outside changes Signed-off-by: Jonas Assisted-by: ClaudeCode:claude-opus-5-5 --- .../outside-change.spec.ts | 91 +++++++++++++++++++ playwright/support/fixtures/User.ts | 16 ++++ 2 files changed, 107 insertions(+) create mode 100644 playwright/e2e/synchronize-content/outside-change.spec.ts diff --git a/playwright/e2e/synchronize-content/outside-change.spec.ts b/playwright/e2e/synchronize-content/outside-change.spec.ts new file mode 100644 index 00000000000..9271fbe2057 --- /dev/null +++ b/playwright/e2e/synchronize-content/outside-change.spec.ts @@ -0,0 +1,91 @@ +/** + * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: AGPL-3.0-or-later + */ + +import { expect, mergeTests } from '@playwright/test' +import { test as editorTest } from '../../support/fixtures/editor.ts' +import { test as randomUserTest } from '../../support/fixtures/random-user.ts' +import { test as uploadFileTest } from '../../support/fixtures/upload-file.ts' + +const test = mergeTests(editorTest, randomUserTest, uploadFileTest) + +test.beforeEach(async ({ open }) => { + await open() +}) + +const resolutionVariants = [ + { + source: 'local', + buttonName: /Keep my local changes and overwrite the remote version/, + headingName: 'Hello world', + }, + { + source: 'server', + buttonName: /Discard my local changes and use the remote version/, + headingName: 'Good bye', + }, +] + +resolutionVariants.forEach(({ source, buttonName, headingName }) => { + test(`[conflict] saving after the file was changed outside of Nextcloud - resolve with ${source} version`, async ({ + container, + editor, + file, + page, + reader, + user, + }) => { + await expect(editor.el).toBeVisible() + const pushPromise = page.waitForRequest(/push/) + await editor.typeHeading('Hello world') + await pushPromise + await user.writeFileOutsideNextcloud({ name: file.name, content: '## Good bye' }) + // Clicking the save indicator would force the save and skip the conflict check. + await page.keyboard.press('ControlOrMeta+s') + + // Verify both versions are shown + await expect(reader.getHeading({ name: 'Good bye' })).toBeVisible({ + timeout: 10_000, + }) + await expect(editor.getHeading({ name: 'Hello world' })).toBeVisible() + + // Resolve conflict + await container.getButton({ name: buttonName }).click() + await expect(reader.el).not.toBeVisible() + await expect(editor.getHeading({ name: headingName })).toBeVisible() + await expect(async () => { + expect(await file.getContent()).toContain(headingName) + }).toPass() + }) +}) + +test('reopening after the file was changed outside of Nextcloud shows the new content', async ({ + close, + container, + editor, + file, + page, + reader, + user, +}) => { + await expect(editor.el).toBeVisible() + const pushPromise = page.waitForRequest(/push/) + await editor.typeHeading('Hello world') + await pushPromise + await editor.saveIndicator.click() + await expect(editor.saveIndicator).not.toHaveAccessibleName(/Unsaved changes/) + await close() + + await user.writeFileOutsideNextcloud({ name: file.name, content: '## Good bye' }) + await file.open() + + await expect(editor.getHeading({ name: 'Good bye' })).toBeVisible({ + timeout: 10_000, + }) + await expect(editor.getHeading({ name: 'Hello world' })).not.toBeVisible() + await expect(reader.el).not.toBeVisible() + await expect(container.getButton({ + name: /Keep my local changes and overwrite the remote version/, + })).not.toBeVisible() +}) diff --git a/playwright/support/fixtures/User.ts b/playwright/support/fixtures/User.ts index d451847d101..4d0f3ad0c5d 100644 --- a/playwright/support/fixtures/User.ts +++ b/playwright/support/fixtures/User.ts @@ -6,6 +6,7 @@ import type { User as Account } from '@nextcloud/e2e-test-server' import type { Page } from '@playwright/test' +import { runExec } from '@nextcloud/e2e-test-server/docker' import { uploadFile } from './Node.ts' export class User { @@ -22,4 +23,19 @@ export class User { uploadFile(upload: Omit[0], 'owner'>) { return uploadFile({ ...upload, owner: this }) } + + /** + * Write a file directly to the user's storage on the server, bypassing Nextcloud + * like a change on external storage would. + * + * @param options options for the write + * @param options.name Name of the file + * @param options.content File content + */ + async writeFileOutsideNextcloud({ name, content }: { name: string, content: string }) { + await runExec( + ['sh', '-c', 'printf "%s" "$CONTENT" > "data/$OWNER/files/$NAME"'], + { env: [`CONTENT=${content}`, `OWNER=${this.account.userId}`, `NAME=${name}`] }, + ) + } }