diff --git a/playwright.config.ts b/playwright.config.ts index 6d510c8d68d..8fbd1516ed9 100644 --- a/playwright.config.ts +++ b/playwright.config.ts @@ -3,24 +3,40 @@ * SPDX-License-Identifier: AGPL-3.0-or-later */ +import type { ReporterDescription } from '@playwright/test' + import { defineConfig, devices } from '@playwright/test' +/** + * Used locally - i.e. if `CI` is not set as an environment variable. + */ +const LOCAL_CONFIG = { + // Just the html report with the traces + reporter: 'list', +} as const + +/** + * Used on CI - i.e. if `CI` is set as an environment variable. + */ +const CI_CONFIG = { + // ensure no `test.only` is left in the code causing false positives + forbidOnly: true, + // blob (so we can merge reports and download them for inspection), + // dot (so we have a quick overview in the logs while the tests are running) + // github (to have annotations in the PR) + reporter: [['blob'], ['line'], ['github']] as ReporterDescription[], + retries: 1, + timeout: 45_000, + // we shard to speed up the tests so no parallelism in workers + workers: 1, +} as const + /** * See https://playwright.dev/docs/test-configuration. */ export default defineConfig({ testDir: './playwright', - // ensure no `test.only` is left in the code causing false positives - forbidOnly: !!process.env.CI, - // retry on CI only - retries: process.env.CI ? 1 : 0, - // we shard on CI to speed up the tests so no parallelism in workers - workers: process.env.CI ? 1 : undefined, - // on CI we want to have blob (so we can merge reports and download them for inspection), - // line (so we have a quick overview in the logs while the tests are running) - // github (to have annotations in the PR) - // locally we just want the html report with the traces - reporter: process.env.CI ? [['blob'], ['line'], ['github']] : 'list', + ...(process.env.CI ? CI_CONFIG : LOCAL_CONFIG), use: { // Base URL to use in actions like `await page.goto('./')`. baseURL: process.env.baseURL ?? 'http://localhost:8089/index.php/', diff --git a/playwright/e2e/autosave.spec.ts b/playwright/e2e/autosave.spec.ts index 328183e4a9f..46ed77deebf 100644 --- a/playwright/e2e/autosave.spec.ts +++ b/playwright/e2e/autosave.spec.ts @@ -14,39 +14,55 @@ const test = mergeTests(editorTest, offlineTest, uploadFileTest) // we cannot run tests in parallel. test.describe.configure({ mode: 'serial' }) +// Files were created 10 seconds ago so there's no throttling to begin with. +test.use({ mtime: Date.now() / 1000 - 10 }) + test.beforeEach(async ({ open }) => { await open() }) -test('saves after 30 seconds', async ({ editor, page }) => { - await page.clock.install() +test('saves after 1 second', async ({ editor }) => { await expect(editor.el).toBeVisible() await editor.typeHeading('Hello world') await expect(editor.saveIndicator).toHaveAccessibleName(/Unsaved changes/) - await page.clock.fastForward(30_000) await expect(editor.saveIndicator).not.toHaveAccessibleName(/Unsaved changes/) // TODO: Why does this not work? await expect(await file.getContent()).toBe('## Hello world') }) -test('saves after being disconnected for 20 sec.', async ({ +/* + * 1 second autosave debounce + * 10 seconds waiting for server to be ready again + * 1 second for the save request + */ +test('saves again within 12 seconds', async ({ editor }) => { + test.slow() + await expect(editor.el).toBeVisible() + await editor.typeHeading('Hello') + await expect(editor.saveIndicator).toHaveAccessibleName(/Unsaved changes/) + await expect(editor.saveIndicator).not.toHaveAccessibleName(/Unsaved changes/) + await editor.type(' again') + await expect(editor.saveIndicator).toHaveAccessibleName(/Unsaved changes/) + await expect(editor.saveIndicator).not.toHaveAccessibleName(/Unsaved changes/, { timeout: 12_000 }) +}) + +test('saves after being disconnected for 5 sec.', async ({ editor, - page, setOffline, setOnline, }) => { - await page.clock.install() await expect(editor.el).toBeVisible() - await editor.typeHeading('Hello world') + await editor.typeHeading('Hello') + await expect(editor.saveIndicator).not.toHaveAccessibleName(/Unsaved changes/) + await editor.type(' again') await expect(editor.saveIndicator).toHaveAccessibleName(/Unsaved changes/) await setOffline() - await page.clock.fastForward(20_000) + await new Promise((resolve) => setTimeout(resolve, 5_000)) await setOnline() - await page.clock.fastForward(20_000) - await expect(editor.saveIndicator).not.toHaveAccessibleName(/Unsaved changes/) - // TODO: Why does this not work? await expect(await file.getContent()).toBe('## Hello world') + await expect(editor.saveIndicator).toHaveAccessibleName(/Unsaved changes/) + await expect(editor.saveIndicator).not.toHaveAccessibleName(/Unsaved changes/, { timeout: 10_000 }) }) -test('saves after being disconnected for 2 minutes', async ({ +test('saves after being disconnected for 2 minutes.', async ({ editor, page, setOffline, @@ -54,12 +70,19 @@ test('saves after being disconnected for 2 minutes', async ({ }) => { await page.clock.install() await expect(editor.el).toBeVisible() - await editor.typeHeading('Hello world') + await editor.typeHeading('Hello') + await expect(editor.saveIndicator).toHaveAccessibleName(/Unsaved changes/) + await expect(editor.saveIndicator).not.toHaveAccessibleName(/Unsaved changes/) + await editor.type(' again') await expect(editor.saveIndicator).toHaveAccessibleName(/Unsaved changes/) await setOffline() - await page.clock.fastForward(120_000) + // Wait long enough for the server throttling to be over. + await new Promise((resolve) => setTimeout(resolve, 10_000)) + await page.clock.fastForward(110_000) await setOnline() - await page.clock.fastForward(40_000) + await expect(editor.offlineState).not.toBeVisible() + await expect(editor.saveIndicator).toHaveAccessibleName(/Unsaved changes/) + // Be sure to trigger at least one autosave + await page.clock.fastForward(15_000) await expect(editor.saveIndicator).not.toHaveAccessibleName(/Unsaved changes/) - // TODO: Why does this not work? await expect(await file.getContent()).toBe('## Hello world') }) diff --git a/playwright/support/fixtures/upload-file.ts b/playwright/support/fixtures/upload-file.ts index 5dce3417ee8..2ff391ea57c 100644 --- a/playwright/support/fixtures/upload-file.ts +++ b/playwright/support/fixtures/upload-file.ts @@ -12,6 +12,7 @@ export interface UploadFileFixture { file: Node fileName: string fileContent: string + mtime?: number oldVersions: { content?: string, mtime: number }[] open: () => Promise close: () => Promise @@ -25,15 +26,16 @@ export interface UploadFileFixture { export const test = base.extend({ fileContent: ['', { option: true }], fileName: ['empty.md', { option: true }], + mtime: [undefined, { option: true }], oldVersions: [[], { option: true }], - file: async ({ fileContent, fileName, oldVersions, user }, use) => { + file: async ({ fileContent, fileName, oldVersions, mtime, user }, use) => { const uploadVersion = (opts: { content?: string, mtime?: number }) => user.uploadFile({ name: fileName, ...opts }) for (const version of oldVersions) { await uploadVersion(version) } - const file = await uploadVersion({ content: fileContent }) + const file = await uploadVersion({ content: fileContent, mtime }) await use(file) }, diff --git a/src/apis/save.ts b/src/apis/save.ts index 3a3ab8f3636..554861736b3 100644 --- a/src/apis/save.ts +++ b/src/apis/save.ts @@ -12,16 +12,19 @@ import axios from '@nextcloud/axios' import { generateUrl } from '@nextcloud/router' import { unref } from 'vue' -interface SaveData { +export interface SaveData { version: number autosaveContent: string documentState: string +} + +export interface SaveOptions { force: boolean manualSave: boolean } interface SaveResponse { - data: Document + data: { document: Document } } /** @@ -32,7 +35,7 @@ interface SaveResponse { */ export function save( connection: ShallowRef | Connection, - data: SaveData, + data: SaveData & SaveOptions, ): Promise { const con = unref(connection) const pub = con.shareToken ? '/public' : '' @@ -61,7 +64,7 @@ export function save( */ export function saveViaSendBeacon( connection: Connection, - data: Omit, + data: SaveData, ): boolean { const con = unref(connection) const pub = con.shareToken ? '/public' : '' diff --git a/src/components/CollaborativeEditor.vue b/src/components/CollaborativeEditor.vue index 35345dc44e2..6651cba52e2 100644 --- a/src/components/CollaborativeEditor.vue +++ b/src/components/CollaborativeEditor.vue @@ -318,11 +318,12 @@ export default defineComponent({ ? () => createMarkdownSerializer(editor.schema).serialize(editor.state.doc) : () => serializePlainText(editor.state.doc) - const { saveService } = provideSaveService( + const { document, saveService } = provideSaveService( connection, syncService, serialize, ydoc, + setDirty, ) const syncProvider = shallowRef(null) @@ -343,6 +344,7 @@ export default defineComponent({ clearIndexedDb, connection, dirty, + document, editor, editorReady, el, @@ -371,7 +373,6 @@ export default defineComponent({ return { IDLE_TIMEOUT, - document: null, fileNode: null, idle: false, @@ -589,8 +590,9 @@ export default defineComponent({ bus.on('error', this.onError) bus.on('stateChange', this.onStateChange) bus.on('idle', this.onIdle) - bus.on('save', this.onSave) bus.on('permissionChange', this.onPermissionChange) + this.saveService.bus.on('error', this.onError) + this.saveService.bus.on('save', this.onSave) }, unlistenSyncServiceEvents() { @@ -601,8 +603,9 @@ export default defineComponent({ bus.off('error', this.onError) bus.off('stateChange', this.onStateChange) bus.off('idle', this.onIdle) - bus.off('save', this.onSave) bus.off('permissionChange', this.onPermissionChange) + this.saveService.bus.off('error', this.onError) + this.saveService.bus.off('save', this.onSave) }, reconnect() { @@ -614,8 +617,7 @@ export default defineComponent({ this.idle = false }, - onOpened({ document, session, content, documentState, readOnly }) { - this.document = document + onOpened({ session, content, documentState, readOnly }) { this.readOnly = readOnly this.editMode = !readOnly && !this.openReadOnlyEnabled this.hasConnectionIssue = false @@ -657,18 +659,14 @@ export default defineComponent({ // Save and push unsaved changes from offline editing session. Promise.all([this.whenSynced, this.editorReady]).then(() => { if (this.dirty) { - this.saveService - .save() - .catch((err) => logger.error('Failed to save offline changes', { err })) + // the update will trigger an autosave this.syncProvider.sendUpdateFromDoc('offline', this.ydoc) } }) this.updateUser(session) }, - onChange({ document }) { - this.document = document - + onChange() { this.syncError = null this.setEditable(this.editMode) }, @@ -690,7 +688,7 @@ export default defineComponent({ }) }, - onSync({ document }) { + onSync() { this.hasConnectionIssue = this.syncService.backend.fetcher === 0 || !this.syncProvider?.wsconnected @@ -702,9 +700,6 @@ export default defineComponent({ this.$nextTick(() => { this.$emit('syncService:sync') }) - if (document) { - this.document = document - } }, onError({ type, data }) { @@ -758,17 +753,6 @@ export default defineComponent({ } this.$emit('ready') } - if (Object.hasOwn(state, 'dirty')) { - if (state.dirty) { - // ignore initial loading and other automated changes before first user change - if (this.editor.can().undo() || this.editor.can().redo()) { - this.setDirty(state.dirty) - this.saveService.autosave() - } - } else { - this.setDirty(state.dirty) - } - } }, onIdle() { diff --git a/src/composables/useSaveService.ts b/src/composables/useSaveService.ts index dccfc1c0853..7d1b3bfa9d0 100644 --- a/src/composables/useSaveService.ts +++ b/src/composables/useSaveService.ts @@ -5,10 +5,11 @@ import type { InjectionKey, ShallowRef } from 'vue' import type { Doc } from 'yjs' -import type { SyncService } from '../services/SyncService.ts' +import type { SaveData } from '../apis/save.ts' +import type { Document, SyncService } from '../services/SyncService.ts' import type { Connection } from './useConnection.ts' -import { inject, provide } from 'vue' +import { computed, inject, onUnmounted, provide, ref, watch } from 'vue' import { getDocumentState } from '../helpers/yjs.ts' import { SaveService } from '../services/SaveService.ts' @@ -20,21 +21,87 @@ const saveServiceKey = Symbol('text:save') as InjectionKey * @param syncService mostly used for the event bus and events * @param serialize to extract the document markdown content * @param ydoc to extract the document state from + * @param setDirty set the dirty state for the editor */ export function provideSaveService( connection: ShallowRef, syncService: SyncService, serialize: () => string, ydoc: Doc, + setDirty: (val: boolean) => Promise, ) { + const document = ref() + + /** + * Get the data for the save request + */ + function getSaveData(): SaveData { + return { + version: syncService.version, + autosaveContent: serialize(), + documentState: getDocumentState(ydoc), + } + } + const saveService = new SaveService({ connection, - syncService, - serialize, - getDocumentState: () => getDocumentState(ydoc), + document, + getSaveData, + }) + + syncService.bus.on('changesPushed', saveService.autosave) + syncService.bus.on('close', saveService.clear) + onUnmounted(() => { + syncService.bus.off('changesPushed', saveService.autosave) + syncService.bus.off('close', saveService.clear) + }) + + /** + * Update the document ref based on the event provided + * + * @param event that triggered the update + * @param event.document latest state of the document + */ + function updateDocument(event: { document: Document }) { + // Limit lastSavedVersionTime to now. No saving from the future. + const lastSavedVersionTime = Math.min( + event.document.lastSavedVersionTime, + Math.ceil(Date.now() / 1000), + ) + document.value = { + ...event.document, + lastSavedVersionTime, + } + } + syncService.bus.on('opened', updateDocument) + syncService.bus.on('change', updateDocument) + saveService.bus.on('save', updateDocument) + onUnmounted(() => { + syncService.bus.off('opened', updateDocument) + syncService.bus.off('change', updateDocument) + saveService.bus.off('save', updateDocument) }) + + const versionWithChanges = ref(0) + /** + * Update the tracked version based on the one in the event + * + * @param event that triggered the update + * @param event.version with changes pushed to the server + */ + function updateVersionWithChanges(event: { version: number }) { + versionWithChanges.value = Math.max(event.version, versionWithChanges.value) + } + syncService.bus.on('changesPushed', updateVersionWithChanges) + onUnmounted(() => { + syncService.bus.off('changesPushed', updateVersionWithChanges) + }) + + const dirty = computed(() => (document.value?.lastSavedVersion ?? 0) < versionWithChanges.value) + watch(dirty, setDirty) + provide(saveServiceKey, saveService) - return { saveService } + return { document, saveService } } /** diff --git a/src/composables/useSyncService.ts b/src/composables/useSyncService.ts index 3f216af3179..3a612f08c46 100644 --- a/src/composables/useSyncService.ts +++ b/src/composables/useSyncService.ts @@ -26,6 +26,7 @@ export function provideSyncService( openConnection, }) provide(syncServiceKey, syncService) + return { syncService } } diff --git a/src/services/PollingBackend.ts b/src/services/PollingBackend.ts index 2e8c4181f42..ff6fe94e826 100644 --- a/src/services/PollingBackend.ts +++ b/src/services/PollingBackend.ts @@ -140,23 +140,23 @@ class PollingBackend { } _handleResponse({ data }: { data: PollData }) { - const { document, sessions } = data + const { document, readOnly, sessions, steps } = data this.#fetchRetryCounter = 0 - if (data.readOnly !== undefined && data.readOnly !== this.#readOnly) { - this.#readOnly = data.readOnly + if (readOnly !== undefined && readOnly !== this.#readOnly) { + this.#readOnly = readOnly this.#syncService.bus.emit('permissionChange', { readOnly: this.#readOnly, }) - if (data.readOnly) { + if (readOnly) { this.maximumReadOnlyTimer() } } this.#syncService.bus.emit('change', { document, sessions }) - this.#syncService.receiveSteps(data) + this.#syncService.receiveSteps({ sessions, steps }) - if (data.steps.length === 0) { + if (steps.length === 0) { if (!this.#initialLoadingFinished) { this.#initialLoadingFinished = true } diff --git a/src/services/SaveService.ts b/src/services/SaveService.ts index f0088ded779..0992ea12d71 100644 --- a/src/services/SaveService.ts +++ b/src/services/SaveService.ts @@ -3,77 +3,91 @@ * SPDX-License-Identifier: AGPL-3.0-or-later */ -import type { ShallowRef } from 'vue' +import type { Ref, ShallowRef } from 'vue' +import type { SaveData } from '../apis/save.ts' import type { Connection } from '../composables/useConnection.ts' -import type { SyncService } from './SyncService.ts' +import type { Document } from './SyncService.ts' import { showError } from '@nextcloud/dialogs' import debounce from 'debounce' +import mitt from 'mitt' import { save, saveViaSendBeacon } from '../apis/save.ts' import { logger } from '../helpers/logger.js' import { ERROR_TYPE } from './SyncService.ts' -/** - * Interval to save the serialized document and the document state - * - * time in ms - */ -const AUTOSAVE_INTERVAL = 30000 +// Time constants in seconds: +// Only autosave after 1 second typing breaks +const AUTOSAVE_DEBOUNCE = 1 +// Server only accepts auutosaves every 10 seconds +const SERVER_AUTOSAVE_INTERVAL = 10 + +type ErrorType = (typeof ERROR_TYPE)[keyof typeof ERROR_TYPE] + +export declare type EventTypes = { + /* error */ + error: { type: ErrorType, data?: object } + + /* Emitted after successful save */ + save: { document: Document } +} class SaveService { + bus = mitt() connection: ShallowRef - syncService - serialize - getDocumentState + document: Ref + lastSaveAttempt = 0 + pendingAutosave = 0 + getSaveData autosave + clear constructor({ connection, - syncService, - serialize, - getDocumentState, + document, + getSaveData, }: { connection: ShallowRef - syncService: SyncService - serialize: () => string - getDocumentState: () => string + document: Ref + getSaveData: () => SaveData }) { this.connection = connection - this.syncService = syncService - this.serialize = serialize - this.getDocumentState = getDocumentState - this.autosave = debounce(this._autosave.bind(this), AUTOSAVE_INTERVAL) - this.syncService.bus.on('close', () => { - this.autosave.clear() - }) - } - - get version() { - return this.syncService.version - } - - get emit() { - return this.syncService.bus.emit + this.document = document + this.getSaveData = getSaveData + this.autosave = debounce(this._autosave.bind(this), AUTOSAVE_DEBOUNCE * 1000) + this.clear = this.clearAutosave.bind(this) } + /** + * Save the current state + * + * @param options for saving + * @param options.force force save for handling conflicts + * @param options.manualSave user initiated the saving - not autosave + * @return true on success, false if autosave was throttled by the server + */ async save({ force = false, manualSave = true } = {}) { logger.debug('[SaveService] saving', { force, manualSave }) if (!this.connection.value) { logger.warn('Could not save due to missing connection') return } + const data = this.getSaveData() try { + this.lastSaveAttempt = Date.now() const response = await save(this.connection.value, { - version: this.version, - autosaveContent: this.serialize(), - documentState: this.getDocumentState(), + ...data, force, manualSave, }) - this.emit('stateChange', { dirty: false }) + // update the document - even if the save was throttled + this.bus.emit('save', response.data) + if (response.data.document.lastSavedVersion < data.version) { + logger.debug('[SaveService] Server throttled save request.', { response }) + return false + } logger.debug('[SaveService] saved', { response }) - this.emit('save', response.data) - this.autosave.clear() + this.clearAutosave() + return true } catch (e) { logger.error('Failed to save document.', { error: e }) const response = ( @@ -84,7 +98,7 @@ class SaveService { return } if (response?.status === 412) { - this.emit('error', { + this.bus.emit('error', { type: ERROR_TYPE.LOAD_ERROR, data: response, }) @@ -100,11 +114,7 @@ class SaveService { if (!this.connection.value) { return } - const success = saveViaSendBeacon(this.connection.value, { - version: this.version, - autosaveContent: this.serialize(), - documentState: this.getDocumentState(), - }) + const success = saveViaSendBeacon(this.connection.value, this.getSaveData()) if (success) { logger.debug('[SaveService] saved using sendBeacon') } @@ -115,11 +125,37 @@ class SaveService { } _autosave() { - return this.save({ manualSave: false }).catch((error) => { - logger.error('Failed to autosave document.', { error }) - // retry in 30 seconds - this.autosave() - }) + const now = Date.now() + const nextSaveAttempt = this.lastSaveAttempt + SERVER_AUTOSAVE_INTERVAL * 1000 + // Server won't accept autosaves yet + if (now < nextSaveAttempt) { + if (!this.pendingAutosave) { + const wait = nextSaveAttempt - now + logger.debug(`Just saved, will try again in ${Math.ceil(wait)} seconds.`) + this.pendingAutosave = window.setTimeout(this.autosave, wait) + } + return + } + logger.debug('Autosaving') + this.save({ manualSave: false }) + .then((saved) => { + // server did not save due to throttling + if (saved === false) { + this.autosave() + } + }) + .catch((error) => { + logger.error('Failed to autosave document.', { error }) + this.autosave() + }) + } + + clearAutosave() { + this.autosave.clear() + if (this.pendingAutosave) { + window.clearTimeout(this.pendingAutosave) + this.pendingAutosave = 0 + } } } diff --git a/src/services/SyncService.ts b/src/services/SyncService.ts index 5a6f7c06e51..11ac87f4e1a 100644 --- a/src/services/SyncService.ts +++ b/src/services/SyncService.ts @@ -110,10 +110,13 @@ export declare type EventTypes = { opened: OpenData /* received new steps */ - sync: { document?: object, steps: Step[] } + sync: { steps: Step[] } /* state changed (dirty) */ - stateChange: { initialLoading?: boolean, dirty?: boolean } + stateChange: { initialLoading?: boolean } + + /* local yjs changes have been pushed to the server */ + changesPushed: { version: number } /* error */ error: { type: ErrorType, data?: object } @@ -121,9 +124,6 @@ export declare type EventTypes = { /* Events for session and document meta data */ change: { sessions: Session[], document: Document } - /* Emitted after successful save */ - save: object - /* Emitted once a document becomes idle */ idle: void @@ -232,9 +232,7 @@ class SyncService { this.#sending = true clearInterval(this.#sendIntervalId) this.#sendIntervalId = undefined - if (this.#outbox.hasUpdate) { - this.bus.emit('stateChange', { dirty: true }) - } + const hadUpdate = this.#outbox.hasUpdate if (!this.hasActiveConnection()) { return } @@ -257,6 +255,11 @@ class SyncService { this.#sending = false if (steps?.length > 0) { this.receiveSteps({ steps }) + if (hadUpdate) { + // this.version has been increased in receiveSteps + this.bus.emit('changesPushed', { version: this.version }) + logger.debug('changesPushed', { version: this.version }) + } } }) .catch((err) => { @@ -294,17 +297,14 @@ class SyncService { receiveSteps({ steps, - document, sessions = [], }: { steps: Step[] - document?: object sessions?: Session[] }) { const versionAfter = Math.max(this.version, ...steps.map((s) => s.version)) this.bus.emit('sync', { steps: [...awarenessSteps(sessions), ...steps], - document, }) if (this.version < versionAfter) { // Steps up to version where emitted but it looks like they were not processed.