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 7436e48b1e0..0d44ebe69d8 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 287c3934abd..d6984032280 100644 --- a/playwright/support/fixtures/upload-file.ts +++ b/playwright/support/fixtures/upload-file.ts @@ -11,6 +11,7 @@ export interface UploadFileFixture { file: Node fileName: string fileContent: string + mtime?: number oldVersions: { content?: string, mtime: number }[] open: () => Promise close: () => Promise @@ -24,6 +25,7 @@ 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) => { @@ -33,7 +35,7 @@ export const test = base.extend({ 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 45e87f910f7..6b9cd4944ee 100644 --- a/src/apis/save.ts +++ b/src/apis/save.ts @@ -10,16 +10,19 @@ import { unref, type ShallowRef } from 'vue' import type { Connection } from '../composables/useConnection' import type { Document } from '../services/SyncService' -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 } } /** @@ -29,7 +32,7 @@ interface SaveResponse { */ export function save( connection: ShallowRef | Connection, - data: SaveData, + data: SaveData & SaveOptions, ): Promise { const con = unref(connection) const pub = con.shareToken ? '/public' : '' @@ -57,7 +60,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/Editor.vue b/src/components/Editor.vue index 356ce1b4c91..e9ff9fda62b 100644 --- a/src/components/Editor.vue +++ b/src/components/Editor.vue @@ -295,11 +295,12 @@ export default defineComponent({ ) : () => serializePlainText(editor.state.doc) - const { saveService } = provideSaveService( + const { document, saveService } = provideSaveService( connection, syncService, serialize, ydoc, + setDirty, ) const syncProvider = shallowRef(null) @@ -315,6 +316,7 @@ export default defineComponent({ clearIndexedDb, connection, dirty, + document, editor, editorReady, el, @@ -343,7 +345,6 @@ export default defineComponent({ return { IDLE_TIMEOUT, - document: null, fileNode: null, idle: false, @@ -548,8 +549,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() { @@ -560,8 +562,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() { @@ -573,8 +576,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 @@ -628,9 +630,7 @@ export default defineComponent({ this.updateUser(session) }, - onChange({ document }) { - this.document = document - + onChange() { this.syncError = null this.setEditable(this.editMode) }, @@ -664,9 +664,6 @@ export default defineComponent({ this.$nextTick(() => { this.emit('sync-service:sync') }) - if (document) { - this.document = document - } }, onError({ type, data }) { diff --git a/src/composables/useSaveService.ts b/src/composables/useSaveService.ts index 8ef431d8244..2779eb3f97b 100644 --- a/src/composables/useSaveService.ts +++ b/src/composables/useSaveService.ts @@ -20,12 +20,63 @@ export const provideSaveService = ( ) => { 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 } } export const useSaveService = () => { diff --git a/src/composables/useSyncService.ts b/src/composables/useSyncService.ts index 08b4483d1b3..c6038b13a2d 100644 --- a/src/composables/useSyncService.ts +++ b/src/composables/useSyncService.ts @@ -23,6 +23,7 @@ export function provideSyncService( openConnection, }) provide(syncServiceKey, syncService) + return { syncService } } diff --git a/src/services/PollingBackend.ts b/src/services/PollingBackend.ts index e37ce7acea8..0bb049878a5 100644 --- a/src/services/PollingBackend.ts +++ b/src/services/PollingBackend.ts @@ -143,23 +143,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 7d003975481..ad158eca6fb 100644 --- a/src/services/SaveService.ts +++ b/src/services/SaveService.ts @@ -17,62 +17,75 @@ import { ERROR_TYPE, type SyncService } from './SyncService' * * @type {number} time in ms */ -const AUTOSAVE_INTERVAL = 30000 +const AUTOSAVE_DEBOUNCE = 1000 + +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 = ( @@ -83,7 +96,7 @@ class SaveService { return } if (response?.status === 412) { - this.emit('error', { + this.bus.emit('error', { type: ERROR_TYPE.LOAD_ERROR, data: response, }) @@ -111,11 +124,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 586a063604a..4ac1ca72ef5 100644 --- a/src/services/SyncService.ts +++ b/src/services/SyncService.ts @@ -121,9 +121,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 +229,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 +252,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 +294,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.