Skip to content

Commit d5d2cfa

Browse files
AntoLCAntoLC
committed
♻️(frontend) improve useSaveDoc hook
- add tests for useSaveDoc hook - simplify hooks states - reduce useEffect calls
1 parent f2ed8e0 commit d5d2cfa

File tree

3 files changed

+207
-46
lines changed

3 files changed

+207
-46
lines changed

‎src/frontend/apps/impress/jest.config.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ const createJestConfig = nextJest({
99
const config: Config = {
1010
coverageProvider: 'v8',
1111
moduleNameMapper: {
12+
'^@/docs/(.*)$': '<rootDir>/src/features/docs/$1',
1213
'^@/(.*)$': '<rootDir>/src/$1',
1314
},
1415
setupFilesAfterEnv: ['<rootDir>/jest.setup.ts'],

‎src/frontend/apps/impress/src/features/docs/doc-editor/hook/__tests__/useSaveDoc.test.tsx

Lines changed: 185 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,185 @@
1+
import { act, renderHook, waitFor } from '@testing-library/react';
2+
import fetchMock from 'fetch-mock';
3+
import { useRouter } from 'next/router';
4+
import * as Y from 'yjs';
5+
6+
import { AppWrapper } from '@/tests/utils';
7+
8+
import useSaveDoc from '../useSaveDoc';
9+
10+
jest.mock('next/router', () => ({
11+
useRouter: jest.fn(),
12+
}));
13+
14+
jest.mock('@/docs/doc-versioning', () => ({
15+
KEY_LIST_DOC_VERSIONS: 'test-key-list-doc-versions',
16+
}));
17+
18+
jest.mock('@/docs/doc-management', () => ({
19+
useUpdateDoc: jest.requireActual('@/docs/doc-management/api/useUpdateDoc')
20+
.useUpdateDoc,
21+
}));
22+
23+
describe('useSaveDoc', () => {
24+
const mockRouterEvents = {
25+
on: jest.fn(),
26+
off: jest.fn(),
27+
};
28+
29+
beforeEach(() => {
30+
jest.clearAllMocks();
31+
fetchMock.restore();
32+
33+
(useRouter as jest.Mock).mockReturnValue({
34+
events: mockRouterEvents,
35+
});
36+
});
37+
38+
it('should setup event listeners on mount', () => {
39+
const yDoc = new Y.Doc();
40+
const docId = 'test-doc-id';
41+
42+
const addEventListenerSpy = jest.spyOn(window, 'addEventListener');
43+
44+
renderHook(() => useSaveDoc(docId, yDoc, true), {
45+
wrapper: AppWrapper,
46+
});
47+
48+
// Verify router event listeners are set up
49+
expect(mockRouterEvents.on).toHaveBeenCalledWith(
50+
'routeChangeStart',
51+
expect.any(Function),
52+
);
53+
54+
// Verify window event listener is set up
55+
expect(addEventListenerSpy).toHaveBeenCalledWith(
56+
'beforeunload',
57+
expect.any(Function),
58+
);
59+
60+
addEventListenerSpy.mockRestore();
61+
});
62+
63+
it('should not save when canSave is false', async () => {
64+
jest.useFakeTimers();
65+
const yDoc = new Y.Doc();
66+
const docId = 'test-doc-id';
67+
68+
fetchMock.patch('http://test.jest/api/v1.0/documents/test-doc-id/', {
69+
body: JSON.stringify({
70+
id: 'test-doc-id',
71+
content: 'test-content',
72+
title: 'test-title',
73+
}),
74+
});
75+
76+
renderHook(() => useSaveDoc(docId, yDoc, false), {
77+
wrapper: AppWrapper,
78+
});
79+
80+
act(() => {
81+
// Trigger a local update
82+
yDoc.getMap('test').set('key', 'value');
83+
});
84+
85+
act(() => {
86+
// Now advance timers after state has updated
87+
jest.advanceTimersByTime(61000);
88+
});
89+
90+
await waitFor(() => {
91+
expect(fetchMock.calls().length).toBe(0);
92+
});
93+
94+
jest.useRealTimers();
95+
});
96+
97+
it('should save when there are local changes', async () => {
98+
jest.useFakeTimers();
99+
const yDoc = new Y.Doc();
100+
const docId = 'test-doc-id';
101+
102+
fetchMock.patch('http://test.jest/api/v1.0/documents/test-doc-id/', {
103+
body: JSON.stringify({
104+
id: 'test-doc-id',
105+
content: 'test-content',
106+
title: 'test-title',
107+
}),
108+
});
109+
110+
renderHook(() => useSaveDoc(docId, yDoc, true), {
111+
wrapper: AppWrapper,
112+
});
113+
114+
act(() => {
115+
// Trigger a local update
116+
yDoc.getMap('test').set('key', 'value');
117+
});
118+
119+
act(() => {
120+
// Now advance timers after state has updated
121+
jest.advanceTimersByTime(61000);
122+
});
123+
124+
await waitFor(() => {
125+
expect(fetchMock.lastCall()?.[0]).toBe(
126+
'http://test.jest/api/v1.0/documents/test-doc-id/',
127+
);
128+
});
129+
130+
jest.useRealTimers();
131+
});
132+
133+
it('should not save when there are no local changes', async () => {
134+
jest.useFakeTimers();
135+
const yDoc = new Y.Doc();
136+
const docId = 'test-doc-id';
137+
138+
fetchMock.patch('http://test.jest/api/v1.0/documents/test-doc-id/', {
139+
body: JSON.stringify({
140+
id: 'test-doc-id',
141+
content: 'test-content',
142+
title: 'test-title',
143+
}),
144+
});
145+
146+
renderHook(() => useSaveDoc(docId, yDoc, true), {
147+
wrapper: AppWrapper,
148+
});
149+
150+
act(() => {
151+
// Now advance timers after state has updated
152+
jest.advanceTimersByTime(61000);
153+
});
154+
155+
await waitFor(() => {
156+
expect(fetchMock.calls().length).toBe(0);
157+
});
158+
159+
jest.useRealTimers();
160+
});
161+
162+
it('should cleanup event listeners on unmount', () => {
163+
const yDoc = new Y.Doc();
164+
const docId = 'test-doc-id';
165+
const removeEventListenerSpy = jest.spyOn(window, 'removeEventListener');
166+
167+
const { unmount } = renderHook(() => useSaveDoc(docId, yDoc, true), {
168+
wrapper: AppWrapper,
169+
});
170+
171+
unmount();
172+
173+
// Verify router event listeners are cleaned up
174+
expect(mockRouterEvents.off).toHaveBeenCalledWith(
175+
'routeChangeStart',
176+
expect.any(Function),
177+
);
178+
179+
// Verify window event listener is cleaned up
180+
expect(removeEventListenerSpy).toHaveBeenCalledWith(
181+
'beforeunload',
182+
expect.any(Function),
183+
);
184+
});
185+
});

‎src/frontend/apps/impress/src/features/docs/doc-editor/hook/useSaveDoc.tsx

Lines changed: 21 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { useRouter } from 'next/router';
2-
import { useCallback, useEffect, useRef, useState } from 'react';
2+
import { useCallback, useEffect, useState } from 'react';
33
import * as Y from 'yjs';
44

55
import { useUpdateDoc } from '@/docs/doc-management/';
@@ -8,17 +8,16 @@ import { isFirefox } from '@/utils/userAgent';
88

99
import { toBase64 } from '../utils';
1010

11-
const useSaveDoc = (docId: string, doc: Y.Doc, canSave: boolean) => {
11+
const SAVE_INTERVAL = 60000;
12+
13+
const useSaveDoc = (docId: string, yDoc: Y.Doc, canSave: boolean) => {
1214
const { mutate: updateDoc } = useUpdateDoc({
1315
listInvalideQueries: [KEY_LIST_DOC_VERSIONS],
16+
onSuccess: () => {
17+
setIsLocalChange(false);
18+
},
1419
});
15-
const [initialDoc, setInitialDoc] = useState<string>(
16-
toBase64(Y.encodeStateAsUpdate(doc)),
17-
);
18-
19-
useEffect(() => {
20-
setInitialDoc(toBase64(Y.encodeStateAsUpdate(doc)));
21-
}, [doc]);
20+
const [isLocalChange, setIsLocalChange] = useState<boolean>(false);
2221

2322
/**
2423
* Update initial doc when doc is updated by other users,
@@ -29,56 +28,34 @@ const useSaveDoc = (docId: string, doc: Y.Doc, canSave: boolean) => {
2928
const onUpdate = (
3029
_uintArray: Uint8Array,
3130
_pluginKey: string,
32-
updatedDoc: Y.Doc,
31+
_updatedDoc: Y.Doc,
3332
transaction: Y.Transaction,
3433
) => {
35-
if (!transaction.local) {
36-
setInitialDoc(toBase64(Y.encodeStateAsUpdate(updatedDoc)));
37-
}
34+
setIsLocalChange(transaction.local ? true : false);
3835
};
3936

40-
doc.on('update', onUpdate);
37+
yDoc.on('update', onUpdate);
4138

4239
return () => {
43-
doc.off('update', onUpdate);
40+
yDoc.off('update', onUpdate);
4441
};
45-
}, [doc]);
46-
47-
/**
48-
* Check if the doc has been updated and can be saved.
49-
*/
50-
const hasChanged = useCallback(() => {
51-
const newDoc = toBase64(Y.encodeStateAsUpdate(doc));
52-
return initialDoc !== newDoc;
53-
}, [doc, initialDoc]);
54-
55-
const shouldSave = useCallback(() => {
56-
return hasChanged() && canSave;
57-
}, [canSave, hasChanged]);
42+
}, [yDoc]);
5843

5944
const saveDoc = useCallback(() => {
60-
const newDoc = toBase64(Y.encodeStateAsUpdate(doc));
61-
setInitialDoc(newDoc);
45+
if (!canSave || !isLocalChange) {
46+
return;
47+
}
6248

6349
updateDoc({
6450
id: docId,
65-
content: newDoc,
51+
content: toBase64(Y.encodeStateAsUpdate(yDoc)),
6652
});
67-
}, [doc, docId, updateDoc]);
53+
}, [canSave, yDoc, docId, isLocalChange, updateDoc]);
6854

69-
const timeout = useRef<NodeJS.Timeout | null>(null);
7055
const router = useRouter();
7156

7257
useEffect(() => {
73-
if (timeout.current) {
74-
clearTimeout(timeout.current);
75-
}
76-
7758
const onSave = (e?: Event) => {
78-
if (!shouldSave()) {
79-
return;
80-
}
81-
8259
saveDoc();
8360

8461
/**
@@ -94,21 +71,19 @@ const useSaveDoc = (docId: string, doc: Y.Doc, canSave: boolean) => {
9471
};
9572

9673
// Save every minute
97-
timeout.current = setInterval(onSave, 60000);
74+
const timeout = setInterval(onSave, SAVE_INTERVAL);
9875
// Save when the user leaves the page
9976
addEventListener('beforeunload', onSave);
10077
// Save when the user navigates to another page
10178
router.events.on('routeChangeStart', onSave);
10279

10380
return () => {
104-
if (timeout.current) {
105-
clearTimeout(timeout.current);
106-
}
81+
clearInterval(timeout);
10782

10883
removeEventListener('beforeunload', onSave);
10984
router.events.off('routeChangeStart', onSave);
11085
};
111-
}, [router.events, saveDoc, shouldSave]);
86+
}, [router.events, saveDoc]);
11287
};
11388

11489
export default useSaveDoc;

0 commit comments

Comments
 (0)