fix/autosave timer

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/',

playwright/e2e/autosave.spec.ts

@@ -14,52 +14,75 @@ 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,
 	setOnline,
 }) => {
 	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')
 })

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<void>
 	close: () => Promise<void>
@@ -25,15 +26,16 @@ export interface UploadFileFixture {
 export const test = base.extend<UploadFileFixture>({
 	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)
 	},
 

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> | Connection,
-	data: SaveData,
+	data: SaveData & SaveOptions,
 ): Promise<SaveResponse> {
 	const con = unref(connection)
 	const pub = con.shareToken ? '/public' : ''
@@ -61,7 +64,7 @@ export function save(
  */
 export function saveViaSendBeacon(
 	connection: Connection,
-	data: Omit<SaveData, 'force' | 'manualSave'>,
+	data: SaveData,
 ): boolean {
 	const con = unref(connection)
 	const pub = con.shareToken ? '/public' : ''

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() {