diff --git a/src/frontend/apps/impress/src/features/docs/doc-editor/components/BlockNoteEditor.tsx b/src/frontend/apps/impress/src/features/docs/doc-editor/components/BlockNoteEditor.tsx index c3b7e8edd..bf8372a27 100644 --- a/src/frontend/apps/impress/src/features/docs/doc-editor/components/BlockNoteEditor.tsx +++ b/src/frontend/apps/impress/src/features/docs/doc-editor/components/BlockNoteEditor.tsx @@ -22,7 +22,7 @@ import * as Y from 'yjs'; import { Box, TextErrors } from '@/components'; import { useConfig } from '@/core'; import { useCunninghamTheme } from '@/cunningham'; -import { Doc, useProviderStore } from '@/docs/doc-management'; +import { Doc } from '@/docs/doc-management'; import { avatarUrlFromName, useAuth } from '@/features/auth'; import { useAnalytics } from '@/libs/Analytics'; @@ -88,13 +88,12 @@ export const BlockNoteEditor = ({ doc, provider }: BlockNoteEditorProps) => { const { user } = useAuth(); const { setEditor } = useEditorStore(); const { themeTokens } = useCunninghamTheme(); - const { isSynced: isConnectedToCollabServer } = useProviderStore(); const refEditorContainer = useRef(null); const canSeeComment = doc.abilities.comment; // Determine if comments should be visible in the UI const showComments = canSeeComment; - useSaveDoc(doc.id, provider.document, isConnectedToCollabServer); + useSaveDoc(doc.id, provider.document); const { i18n, t } = useTranslation(); const langLocalesBN = !i18n.resolvedLanguage || !(i18n.resolvedLanguage in localesBN) 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 f5eac9eba..878d0ece4 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 @@ -43,7 +43,7 @@ describe('useSaveDoc', () => { const addEventListenerSpy = vi.spyOn(window, 'addEventListener'); - renderHook(() => useSaveDoc(docId, yDoc, true), { + renderHook(() => useSaveDoc(docId, yDoc), { wrapper: AppWrapper, }); @@ -65,19 +65,16 @@ describe('useSaveDoc', () => { it('should save when there are local changes', async () => { vi.useFakeTimers(); const yDoc = new Y.Doc(); - const docId = 'test-doc-id'; + const docId = self.crypto.randomUUID(); - fetchMock.patch( - 'http://test.jest/api/v1.0/documents/test-doc-id/content/', - { - body: JSON.stringify({ - id: 'test-doc-id', - content: 'test-content', - }), - }, - ); + fetchMock.patch(`http://test.jest/api/v1.0/documents/${docId}/content/`, { + body: JSON.stringify({ + id: docId, + content: 'test-content', + }), + }); - renderHook(() => useSaveDoc(docId, yDoc, true), { + renderHook(() => useSaveDoc(docId, yDoc), { wrapper: AppWrapper, }); @@ -96,7 +93,7 @@ describe('useSaveDoc', () => { await waitFor(() => { expect(fetchMock.lastCall()?.[0]).toBe( - 'http://test.jest/api/v1.0/documents/test-doc-id/content/', + `http://test.jest/api/v1.0/documents/${docId}/content/`, ); }); }); @@ -116,7 +113,7 @@ describe('useSaveDoc', () => { }, ); - renderHook(() => useSaveDoc(docId, yDoc, true), { + renderHook(() => useSaveDoc(docId, yDoc), { wrapper: AppWrapper, }); @@ -136,7 +133,7 @@ describe('useSaveDoc', () => { const docId = 'test-doc-id'; const removeEventListenerSpy = vi.spyOn(window, 'removeEventListener'); - const { unmount } = renderHook(() => useSaveDoc(docId, yDoc, true), { + const { unmount } = renderHook(() => useSaveDoc(docId, yDoc), { wrapper: AppWrapper, }); 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 b8e9c69f8..85ac5bc1b 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,22 +2,28 @@ import { useRouter } from 'next/router'; import { useCallback, useEffect, useRef, useState } from 'react'; import * as Y from 'yjs'; -import { KEY_DOC_CONTENT } from '@/docs//doc-management/api/useDocContent'; import { 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 { useIsOffline } from '@/features/service-worker'; import { toBase64 } from '@/utils/string'; import { isFirefox } from '@/utils/userAgent'; const SAVE_INTERVAL = 60000; -export const useSaveDoc = ( - docId: string, - yDoc: Y.Doc, - isConnectedToCollabServer: boolean, -) => { +export const useSaveDoc = (docId: string, yDoc: Y.Doc) => { + /** + * isSynced is more reliable than isConnected in this cases + * because it indicates that the content is fully synchronised + * with the yjs server + */ + const { isSynced: isConnectedToCollabServer } = useProviderStore(); + + const { isOffline } = useIsOffline(); const isSavingRef = useRef(false); const { mutate: updateDocContent } = useDocContentUpdate({ - listInvalidQueries: [KEY_LIST_DOC_VERSIONS, KEY_DOC_CONTENT], + listInvalidQueries: [KEY_LIST_DOC_VERSIONS], + isOptimistic: isOffline, // Enable optimistic updates when offline, to update the cache immediately onSuccess: () => { isSavingRef.current = false; setIsLocalChange(false); 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 ff73d0f6e..23cb7402e 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 @@ -3,14 +3,16 @@ import { useMutation, useQueryClient, } from '@tanstack/react-query'; +import { validate as uuidValidate } from 'uuid'; import { APIError, errorCauses, fetchAPI } from '@/api'; import { Doc } from '../types'; import { KEY_CAN_EDIT } from './useDocCanEdit'; +import { KEY_DOC_CONTENT } from './useDocContent'; -interface UpdateDocContentParams { +export interface UpdateDocContentParams { id: Doc['id']; content: string; // Base64 encoded content websocket?: boolean; @@ -21,6 +23,10 @@ export const updateDocContent = async ({ content, websocket, }: UpdateDocContentParams): Promise => { + if (!uuidValidate(id)) { + throw new Error(`Invalid doc id in updateDocContent: ${id}`); + } + const response = await fetchAPI(`documents/${id}/content/`, { method: 'PATCH', body: JSON.stringify({ @@ -42,6 +48,7 @@ type UseDocContentUpdate = UseMutationOptions< APIError, UpdateDocContentParams > & { + isOptimistic?: boolean; listInvalidQueries?: string[]; }; @@ -50,7 +57,37 @@ export function useDocContentUpdate(queryConfig?: UseDocContentUpdate) { return useMutation({ mutationFn: updateDocContent, ...queryConfig, + onMutate: (variables) => { + /** + * If optimistic, we update the content cache immediately with the new content + * It is useful when we are in offline mode because the onSuccess is not always triggered. + */ + if (queryConfig?.isOptimistic) { + const previousContent = queryClient.getQueryData([ + KEY_DOC_CONTENT, + { id: variables.id }, + ]); + + queryClient.setQueryData( + [KEY_DOC_CONTENT, { id: variables.id }], + variables.content, + ); + + return { previousContent }; + } + }, onSuccess: (data, variables, onMutateResult, context) => { + if (!queryConfig?.isOptimistic) { + /** + * If not optimistic, we need to update the content cache with the new content returned + * from the server + */ + queryClient.setQueryData( + [KEY_DOC_CONTENT, { id: variables.id }], + variables.content, + ); + } + queryConfig?.listInvalidQueries?.forEach((queryKey) => { void queryClient.resetQueries({ queryKey: [queryKey], @@ -62,6 +99,19 @@ export function useDocContentUpdate(queryConfig?: UseDocContentUpdate) { } }, onError: (error, variables, onMutateResult, context) => { + if ( + queryConfig?.isOptimistic && + (onMutateResult as { previousContent: unknown })?.previousContent + ) { + const previousContent = (onMutateResult as { previousContent: unknown }) + .previousContent; + + queryClient.setQueryData( + [KEY_DOC_CONTENT, { id: variables.id }], + previousContent, + ); + } + // If error it means the user is probably not allowed to edit the doc // so we invalidate the canEdit query to update the UI accordingly void queryClient.invalidateQueries({ diff --git a/src/frontend/apps/impress/src/features/docs/doc-management/api/useDuplicateDoc.tsx b/src/frontend/apps/impress/src/features/docs/doc-management/api/useDuplicateDoc.tsx index 87f90a51a..96ff43943 100644 --- a/src/frontend/apps/impress/src/features/docs/doc-management/api/useDuplicateDoc.tsx +++ b/src/frontend/apps/impress/src/features/docs/doc-management/api/useDuplicateDoc.tsx @@ -17,7 +17,6 @@ import { toBase64 } from '@/utils/string'; import { useProviderStore } from '../stores'; import { Doc } from '../types'; -import { KEY_DOC_CONTENT } from './useDocContent'; import { useDocContentUpdate } from './useDocContentUpdate'; import { KEY_LIST_DOC } from './useDocs'; @@ -64,7 +63,7 @@ export function useDuplicateDoc(options?: DuplicateDocOptions) { const { provider } = useProviderStore(); const { mutateAsync: updateDocContent } = useDocContentUpdate({ - listInvalidQueries: [KEY_LIST_DOC_VERSIONS, KEY_DOC_CONTENT], + listInvalidQueries: [KEY_LIST_DOC_VERSIONS], }); return useMutation({ diff --git a/src/frontend/apps/impress/src/features/docs/doc-management/hooks/useCollaboration.tsx b/src/frontend/apps/impress/src/features/docs/doc-management/hooks/useCollaboration.tsx index d9bbcc4f2..769291de3 100644 --- a/src/frontend/apps/impress/src/features/docs/doc-management/hooks/useCollaboration.tsx +++ b/src/frontend/apps/impress/src/features/docs/doc-management/hooks/useCollaboration.tsx @@ -7,6 +7,7 @@ import { useDocContent, } from '@/docs/doc-management/api/useDocContent'; import { useProviderStore } from '@/docs/doc-management/stores/useProviderStore'; +import { useIsOffline } from '@/features/service-worker/hooks/useOffline'; import { useBroadcastStore } from '@/stores/useBroadcastStore'; import { KEY_DOC } from '../api'; @@ -20,10 +21,12 @@ export const useCollaboration = (room: string) => { provider, createProvider, destroyProvider, + setReady, isReady, hasLostConnection, resetLostConnection, } = useProviderStore(); + const isOffline = useIsOffline((state) => state.isOffline); const { data: docContent } = useDocContent( { id: room }, { @@ -32,6 +35,17 @@ export const useCollaboration = (room: string) => { }, ); + /** + * When offline, the WebSocket never connects so the provider would stay + * in a non-ready state for a long time. Immediately mark it as ready so + * the editor can render with the cached content. + */ + useEffect(() => { + if (isOffline && provider && !isReady) { + setReady(true); + } + }, [isOffline, isReady, provider, setReady]); + /** * When the provider detects a lost connection, we invalidate the document query to trigger a refetch. * Because it can be because the user has access to the document that are modified diff --git a/src/frontend/apps/impress/src/features/docs/doc-management/stores/useProviderStore.tsx b/src/frontend/apps/impress/src/features/docs/doc-management/stores/useProviderStore.tsx index ca109548b..829d751fb 100644 --- a/src/frontend/apps/impress/src/features/docs/doc-management/stores/useProviderStore.tsx +++ b/src/frontend/apps/impress/src/features/docs/doc-management/stores/useProviderStore.tsx @@ -12,6 +12,7 @@ export interface UseCollaborationStore { initialDoc?: Base64, ) => HocuspocusProvider; destroyProvider: () => void; + setReady: (value: boolean) => void; provider: HocuspocusProvider | undefined; isConnected: boolean; isReady: boolean; @@ -161,5 +162,6 @@ export const useProviderStore = create((set, get) => ({ set(defaultValues); }, + setReady: (value: boolean) => set({ isReady: value }), resetLostConnection: () => set({ hasLostConnection: false }), })); diff --git a/src/frontend/apps/impress/src/features/service-worker/__tests__/ApiPlugin.test.tsx b/src/frontend/apps/impress/src/features/service-worker/__tests__/ApiPlugin.test.tsx index 1962134bc..b4ef36d85 100644 --- a/src/frontend/apps/impress/src/features/service-worker/__tests__/ApiPlugin.test.tsx +++ b/src/frontend/apps/impress/src/features/service-worker/__tests__/ApiPlugin.test.tsx @@ -1,6 +1,7 @@ import { afterEach, describe, expect, it, vi } from 'vitest'; import { RequestSerializer } from '../RequestSerializer'; +import { SyncManager } from '../SyncManager'; import { ApiPlugin } from '../plugins/ApiPlugin'; const mockedGet = vi.fn().mockResolvedValue({}); @@ -169,7 +170,7 @@ describe('ApiPlugin', () => { const apiPlugin = new ApiPlugin({ type: 'content', tableName: 'doc-content', - syncManager: { sync: () => mockedSync() } as any, + syncManager: { sync: () => mockedSync() } as SyncManager, }); mockedGet.mockResolvedValue({ @@ -200,6 +201,7 @@ describe('ApiPlugin', () => { [ { type: 'list', tableName: 'doc-list' }, { type: 'item', tableName: 'doc-item' }, + { type: 'content', tableName: 'doc-content' }, ].forEach(({ type, tableName }) => { it(`checks handlerDidError with type ${type}`, async () => { const requestInit = { @@ -211,7 +213,7 @@ describe('ApiPlugin', () => { const apiPlugin = new ApiPlugin({ type: type as 'list' | 'item' | 'update' | 'create' | 'delete', tableName: tableName as 'doc-list' | 'doc-item', - syncManager: {} as any, + syncManager: {} as SyncManager, }); await apiPlugin.fetchDidFail?.({} as any); @@ -297,6 +299,72 @@ describe('ApiPlugin', () => { expect(response?.status).toBe(200); }); + it(`checks handlerDidError with type content-update`, async () => { + const requestInit = { + request: { + url: 'http://test.jest/documents/123456/content/', + clone: () => mockedClone(), + headers: new Headers({ + 'Content-Type': 'application/json', + }), + arrayBuffer: () => + RequestSerializer.objectToArrayBuffer({ + content: 'test', + }), + json: () => ({ + content: 'test', + }), + } as unknown as Request, + } as any; + + const mockedClone = vi.fn().mockReturnValue(requestInit.request); + + const mockedSync = vi.fn().mockResolvedValue({}); + const apiPlugin = new ApiPlugin({ + type: 'content-update', + syncManager: { + sync: () => mockedSync(), + } as any, + }); + + mockedGet.mockResolvedValue({ + etag: '', + lastModified: '', + content: '', + }); + + await apiPlugin.requestWillFetch?.(requestInit); + await apiPlugin.fetchDidFail?.({} as any); + const response = await apiPlugin.handlerDidError?.(requestInit); + expect(mockedGet).toHaveBeenCalledWith( + 'doc-content', + 'http://test.jest/documents/123456/content/', + ); + + expect(mockedPut).toHaveBeenCalledWith( + 'doc-mutation', + expect.objectContaining({ + key: expect.any(String), + requestData: expect.objectContaining({ + url: 'http://test.jest/documents/123456/content/', + headers: { + 'content-type': 'application/json', + }, + }), + }), + expect.any(String), + ); + expect(mockedPut).toHaveBeenCalledWith( + 'doc-content', + { etag: '', lastModified: '', content: 'test' }, + 'http://test.jest/documents/123456/content/', + ); + + expect(mockedPut).toHaveBeenCalledTimes(2); + expect(mockedClose).toHaveBeenCalled(); + expect(response?.status).toBe(204); + }); + it(`checks handlerDidError with type delete`, async () => { const requestInit = { request: { @@ -346,6 +414,10 @@ describe('ApiPlugin', () => { 'doc-item', 'http://test.jest/documents/123456/', ); + expect(mockedDelete).toHaveBeenCalledWith( + 'doc-content', + 'http://test.jest/documents/123456/content/', + ); expect(mockedGetAllKeys).toHaveBeenCalledWith('doc-list'); expect(mockedGet).toHaveBeenCalledWith( 'doc-list', @@ -437,6 +509,15 @@ describe('ApiPlugin', () => { expect.objectContaining({}), 'http://test.jest/documents/444555/', ); + expect(mockedPut).toHaveBeenCalledWith( + 'doc-content', + expect.objectContaining({ + content: '', + etag: '', + lastModified: '', + }), + 'http://test.jest/documents/444555/content/', + ); expect(mockedPut).toHaveBeenCalledWith( 'doc-list', expect.objectContaining({ @@ -453,7 +534,7 @@ describe('ApiPlugin', () => { 'doc-list', 'http://test.jest/documents/?page=1', ); - expect(mockedPut).toHaveBeenCalledTimes(3); + expect(mockedPut).toHaveBeenCalledTimes(4); expect(mockedClose).toHaveBeenCalled(); expect(response?.status).toBe(201); }); diff --git a/src/frontend/apps/impress/src/features/service-worker/plugins/ApiPlugin.ts b/src/frontend/apps/impress/src/features/service-worker/plugins/ApiPlugin.ts index a44311502..9d56df319 100644 --- a/src/frontend/apps/impress/src/features/service-worker/plugins/ApiPlugin.ts +++ b/src/frontend/apps/impress/src/features/service-worker/plugins/ApiPlugin.ts @@ -2,6 +2,7 @@ import { WorkboxPlugin } from 'workbox-core'; import { Doc, DocsResponse } from '@/docs/doc-management'; import { LinkReach, LinkRole, Role } from '@/docs/doc-management/types'; +import { UpdateDocContentParams } from '@/features/docs/doc-management/api/useDocContentUpdate'; import { DBRequest, DocsDB } from '../DocsDB'; import { RequestSerializer } from '../RequestSerializer'; @@ -13,7 +14,7 @@ interface OptionsReadonly { } interface OptionsMutate { - type: 'update' | 'delete' | 'create'; + type: 'update' | 'delete' | 'create' | 'content-update'; } interface OptionsSync { @@ -130,6 +131,7 @@ export class ApiPlugin implements WorkboxPlugin { requestWillFetch: WorkboxPlugin['requestWillFetch'] = async ({ request }) => { if ( this.options.type === 'update' || + this.options.type === 'content-update' || this.options.type === 'create' || this.options.type === 'delete' ) { @@ -173,6 +175,8 @@ export class ApiPlugin implements WorkboxPlugin { return this.handlerDidErrorDelete(request); case 'update': return this.handlerDidErrorUpdate(request); + case 'content-update': + return this.handlerDidErrorContentUpdate(request); case 'list': case 'item': return this.handlerDidErrorRead(this.options.tableName, request.url); @@ -183,6 +187,21 @@ export class ApiPlugin implements WorkboxPlugin { return Promise.resolve(ApiPlugin.getApiCatchHandler()); }; + private queueMutation = async (request: Request): Promise => { + const requestData = ( + await RequestSerializer.fromRequest(request) + ).toObject(); + const serializeRequest: DBRequest = { + requestData, + key: `${Date.now()}`, + }; + await DocsDB.cacheResponse( + serializeRequest.key, + serializeRequest, + 'doc-mutation', + ); + }; + private handlerDidErrorCreate = async (request: Request) => { if (!this.initialRequest) { return new Response('Request not found', { status: 404 }); @@ -271,12 +290,26 @@ export class ApiPlugin implements WorkboxPlugin { ancestors_link_role: undefined, }; + /** + * Create a new document in the cache with the new id, so the client can use it while offline, + * and it will be updated later when the request will be synced. + */ await DocsDB.cacheResponse( `${request.url}${uuid}/`, newResponse, 'doc-item', ); + /** + * Create an empty content for the new document in the cache, so the client can use it while offline, + * and it will be updated later when the request will be synced. + */ + await DocsDB.cacheResponse( + `${request.url}${uuid}/content/`, + { etag: '', lastModified: '', content: '' }, + 'doc-content', + ); + /** * Add the new entry to the cache list. */ @@ -312,26 +345,14 @@ export class ApiPlugin implements WorkboxPlugin { /** * Queue the request in the cache 'doc-mutation' to sync it later. */ - const requestData = ( - await RequestSerializer.fromRequest(this.initialRequest) - ).toObject(); - - const serializeRequest: DBRequest = { - requestData, - key: `${Date.now()}`, - }; - - await DocsDB.cacheResponse( - serializeRequest.key, - serializeRequest, - 'doc-mutation', - ); + await this.queueMutation(this.initialRequest); /** * Delete item in the cache */ const db = await DocsDB.open(); await db.delete('doc-item', request.url); + await db.delete('doc-content', `${request.url}content/`); /** * Delete entry from the cache list. @@ -378,20 +399,7 @@ export class ApiPlugin implements WorkboxPlugin { /** * Queue the request in the cache 'doc-mutation' to sync it later. */ - const requestData = ( - await RequestSerializer.fromRequest(this.initialRequest) - ).toObject(); - - const serializeRequest: DBRequest = { - requestData, - key: `${Date.now()}`, - }; - - await DocsDB.cacheResponse( - serializeRequest.key, - serializeRequest, - 'doc-mutation', - ); + await this.queueMutation(this.initialRequest); /** * Update the cache item with the new data. @@ -489,4 +497,36 @@ export class ApiPlugin implements WorkboxPlugin { }, }); }; + + /** + * When the content update fails, we save the new content in the cache, and we will sync it later with the SyncManager. + * We return a 204 to the client to say that the update is successful, and we update the content in the cache so the + * client can see the new content while offline. + */ + private handlerDidErrorContentUpdate = async (request: Request) => { + const db = await DocsDB.open(); + const entry = await db.get('doc-content', request.url); + db.close(); + + if (!entry || !this.initialRequest) { + return new Response('Not found', { status: 404 }); + } + + await this.queueMutation(this.initialRequest); + + const bodyMutate = (await this.initialRequest + .clone() + .json()) as Partial; + const newContent = bodyMutate.content ?? entry.content; + await DocsDB.cacheResponse( + request.url, + { etag: '', lastModified: '', content: newContent }, + 'doc-content', + ); + + return new Response(null, { + status: 204, + statusText: 'No Content', + }); + }; } diff --git a/src/frontend/apps/impress/src/features/service-worker/service-worker-api.ts b/src/frontend/apps/impress/src/features/service-worker/service-worker-api.ts index 677c35f7a..1de19733e 100644 --- a/src/frontend/apps/impress/src/features/service-worker/service-worker-api.ts +++ b/src/frontend/apps/impress/src/features/service-worker/service-worker-api.ts @@ -78,6 +78,31 @@ registerRoute( 'GET', ); +/** + * Mutate routes for the content update + * It will save in cache the request if the content update fails, and will retry + * to sync it later with the SyncManager + */ +registerRoute( + ({ url }) => + isApiUrl(url.href) && /\/documents\/[a-z0-9-]+\/content\/$/.test(url.href), + new NetworkOnly({ + plugins: [ + new ApiPlugin({ + type: 'content-update', + syncManager, + }), + new OfflinePlugin(), + ], + }), + 'PATCH', +); + +/** + * Mutate routes for the document update + * It will save in cache the request if the document update fails, and will retry + * to sync it later with the SyncManager + */ registerRoute( ({ url }) => isDocumentApiUrl(url), new NetworkOnly({