diff --git a/CHANGELOG.md b/CHANGELOG.md index 1d0a3adb0..9979420b1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,7 @@ and this project adheres to - 🐛(backend) fix duplicating a document that has no content - 📄(frontend) allowed partially export when MIT #2551 - 🐛(backend) manage async support for Docs custom middleware +- 🐛(frontend) save the doc with a keepalive request when leaving the page ### Removed diff --git a/src/frontend/apps/impress/src/features/docs/doc-editor/hook/__tests__/useSaveDoc.test.tsx b/src/frontend/apps/impress/src/features/docs/doc-editor/hook/__tests__/useSaveDoc.test.tsx index 8ed670d6b..8b055d9a6 100644 --- a/src/frontend/apps/impress/src/features/docs/doc-editor/hook/__tests__/useSaveDoc.test.tsx +++ b/src/frontend/apps/impress/src/features/docs/doc-editor/hook/__tests__/useSaveDoc.test.tsx @@ -1,7 +1,7 @@ import { act, renderHook, waitFor } from '@testing-library/react'; import fetchMock from 'fetch-mock'; import { useRouter } from 'next/router'; -import { Mock, beforeEach, describe, expect, it, vi } from 'vitest'; +import { Mock, afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import * as Y from 'yjs'; import { AppWrapper } from '@/tests/utils'; @@ -38,6 +38,10 @@ describe('useSaveDoc', () => { }); }); + afterEach(() => { + vi.restoreAllMocks(); + }); + it('should setup event listeners on mount', () => { const yDoc = new Y.Doc(); const docId = 'test-doc-id'; @@ -129,6 +133,105 @@ describe('useSaveDoc', () => { vi.useRealTimers(); }); + const setupSavedDoc = async (yDoc: Y.Doc, docId: string) => { + fetchMock.patch(`http://test.jest/api/v1.0/documents/${docId}/content/`, { + body: JSON.stringify({ id: docId, content: 'test-content' }), + }); + + renderHook(() => useSaveDoc(docId, yDoc), { + wrapper: AppWrapper, + }); + + act(() => { + // Trigger a local update so there is something to save + yDoc.getMap('test').set('key', 'value'); + }); + }; + + const dispatchBeforeUnload = () => { + const event = new Event('beforeunload', { cancelable: true }); + act(() => { + window.dispatchEvent(event); + }); + return event; + }; + + it('should save with keepalive when the page is unloading', async () => { + const yDoc = new Y.Doc(); + const docId = self.crypto.randomUUID(); + + await setupSavedDoc(yDoc, docId); + + const event = dispatchBeforeUnload(); + + await waitFor(() => { + expect(fetchMock.callHistory.lastCall()?.url).toBe( + `http://test.jest/api/v1.0/documents/${docId}/content/`, + ); + }); + + expect(fetchMock.callHistory.lastCall()?.options.keepalive).toBe(true); + // The browser owns the request, no need to hold the unload back + expect(event.defaultPrevented).toBe(false); + }); + + it('should not use keepalive when saving without unloading', async () => { + vi.useFakeTimers(); + const yDoc = new Y.Doc(); + const docId = self.crypto.randomUUID(); + + await setupSavedDoc(yDoc, docId); + + act(() => { + vi.advanceTimersByTime(61000); + }); + + vi.useRealTimers(); + + await waitFor(() => { + expect(fetchMock.callHistory.lastCall()?.url).toBe( + `http://test.jest/api/v1.0/documents/${docId}/content/`, + ); + }); + + expect(fetchMock.callHistory.lastCall()?.options.keepalive).toBeFalsy(); + }); + + it('should hold the unload back when the doc is too big for keepalive with firefox', async () => { + const yDoc = new Y.Doc(); + const docId = self.crypto.randomUUID(); + + // Mock Firefox user agent to simulate Firefox behavior + vi.spyOn(navigator, 'userAgent', 'get').mockReturnValue( + 'Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:109.0) Gecko/20100101 Firefox/117.0', + ); + + fetchMock.patch(`http://test.jest/api/v1.0/documents/${docId}/content/`, { + body: JSON.stringify({ id: docId, content: 'test-content' }), + }); + + renderHook(() => useSaveDoc(docId, yDoc), { + wrapper: AppWrapper, + }); + + act(() => { + // Over the 64 KiB keepalive cap once base64 encoded + yDoc.getText('big').insert(0, 'a'.repeat(70 * 1024)); + }); + + const event = dispatchBeforeUnload(); + + await waitFor(() => { + expect(fetchMock.callHistory.lastCall()?.url).toBe( + `http://test.jest/api/v1.0/documents/${docId}/content/`, + ); + }); + + expect(fetchMock.callHistory.lastCall()?.options.keepalive).toBe(false); + // Regular fetch: the unload is held back so the request has time to go out + expect(event.defaultPrevented).toBe(true); + }); + it('should cleanup event listeners on unmount', () => { const yDoc = new Y.Doc(); const docId = 'test-doc-id'; diff --git a/src/frontend/apps/impress/src/features/docs/doc-editor/hook/useSaveDoc.tsx b/src/frontend/apps/impress/src/features/docs/doc-editor/hook/useSaveDoc.tsx index b6ceb0230..e0fbe7496 100644 --- a/src/frontend/apps/impress/src/features/docs/doc-editor/hook/useSaveDoc.tsx +++ b/src/frontend/apps/impress/src/features/docs/doc-editor/hook/useSaveDoc.tsx @@ -2,7 +2,10 @@ import { useRouter } from 'next/router'; import { useCallback, useEffect, useRef, useState } from 'react'; import * as Y from 'yjs'; -import { useDocContentUpdate } from '@/docs/doc-management/api/useDocContentUpdate'; +import { + canKeepaliveContent, + useDocContentUpdate, +} from '@/docs/doc-management/api/useDocContentUpdate'; import { useProviderStore } from '@/docs/doc-management/stores/useProviderStore'; import { KEY_LIST_DOC_VERSIONS } from '@/docs/doc-versioning/api/useDocVersions'; import { COMMENT_UPDATE_ORIGIN } from '@/features/docs/doc-comments/api/DocsThreadStore'; @@ -86,26 +89,41 @@ export const useSaveDoc = (docId: string, yDoc: Y.Doc) => { }; }, [yDoc]); - const saveDoc = useCallback(() => { - if (!isLocalChange || isSavingRef.current) { - return false; - } + /** + * `isSaving` tells whether a request was actually sent, `isKeptAlive` + * whether it was handed over to the browser process (see `keepalive`) and + * will therefore outlive the page. + */ + const saveDoc = useCallback( + ({ isUnloading = false }: { isUnloading?: boolean } = {}) => { + if (!isLocalChange || isSavingRef.current) { + return { isSaving: false, isKeptAlive: false }; + } - isSavingRef.current = true; - updateDocContent({ - id: docId, - content: toBase64(Y.encodeStateAsUpdate(yDoc)), - websocket: isConnectedToCollabServer, - }); + isSavingRef.current = true; + const content = toBase64(Y.encodeStateAsUpdate(yDoc)); + const websocket = isConnectedToCollabServer; + updateDocContent({ + id: docId, + content, + websocket, + keepalive: isUnloading, + }); - return true; - }, [isLocalChange, updateDocContent, docId, yDoc, isConnectedToCollabServer]); + return { + isSaving: true, + isKeptAlive: isUnloading && canKeepaliveContent({ content, websocket }), + }; + }, + [isLocalChange, updateDocContent, docId, yDoc, isConnectedToCollabServer], + ); const router = useRouter(); useEffect(() => { const onSave = (e?: Event) => { - const isSaving = saveDoc(); + const isUnloading = typeof e !== 'undefined' && e.type === 'beforeunload'; + const { isSaving, isKeptAlive } = saveDoc({ isUnloading }); /** * Firefox does not trigger the request every time the user leaves the page. @@ -113,19 +131,24 @@ export const useSaveDoc = (docId: string, yDoc: Y.Doc) => { * So we prevent the default behavior to have the popup asking the user * if he wants to leave the page, by adding the popup, we let the time to the * request to be sent, and intercepted by the service worker (for the offline part). + * + * We do the same for documents too big to be sent with `keepalive`: the + * request is a regular fetch, so it dies with the page unless we hold + * the unload back. */ if ( isSaving && - typeof e !== 'undefined' && + isUnloading && e.preventDefault && - isFirefox() + isFirefox() && + !isKeptAlive ) { e.preventDefault(); } }; // Save every minute - const timeout = setInterval(onSave, SAVE_INTERVAL); + const timeout = setInterval(() => onSave(), SAVE_INTERVAL); // Save when the user leaves the page addEventListener('beforeunload', onSave); // Save when the user navigates to another page diff --git a/src/frontend/apps/impress/src/features/docs/doc-management/api/useDocContentUpdate.tsx b/src/frontend/apps/impress/src/features/docs/doc-management/api/useDocContentUpdate.tsx index 23cb7402e..0cb0e4576 100644 --- a/src/frontend/apps/impress/src/features/docs/doc-management/api/useDocContentUpdate.tsx +++ b/src/frontend/apps/impress/src/features/docs/doc-management/api/useDocContentUpdate.tsx @@ -16,23 +16,50 @@ export interface UpdateDocContentParams { id: Doc['id']; content: string; // Base64 encoded content websocket?: boolean; + /** + * Hand the request over to the browser process so it survives the page + * being torn down. Needed when saving from "beforeunload": a regular fetch + * is aborted with the document, and the server never sees the request. + */ + keepalive?: boolean; } +/** + * The fetch spec caps the body of a keepalive request. Over that limit the + * browser rejects the request outright, so we fall back to a regular fetch. + */ +const KEEPALIVE_MAX_BODY_SIZE = 64 * 1024; + +const buildContentBody = ({ + content, + websocket, +}: Pick) => + JSON.stringify({ + content, + websocket, + }); + +/** Whether the doc is small enough to be saved with a keepalive request. */ +export const canKeepaliveContent = ( + params: Pick, +) => buildContentBody(params).length <= KEEPALIVE_MAX_BODY_SIZE; + export const updateDocContent = async ({ id, content, websocket, + keepalive, }: UpdateDocContentParams): Promise => { if (!uuidValidate(id)) { throw new Error(`Invalid doc id in updateDocContent: ${id}`); } + const body = buildContentBody({ content, websocket }); + const response = await fetchAPI(`documents/${id}/content/`, { method: 'PATCH', - body: JSON.stringify({ - content, - websocket, - }), + body, + keepalive: keepalive && body.length <= KEEPALIVE_MAX_BODY_SIZE, }); if (!response.ok) {