Skip to content

Commit c80b6e9

Browse files
authored
fix(ui): prevent document drawer from remounting on save (#13005)
Supersedes #12992. Partially closes #12975. Right now autosave-enabled documents opened within a drawer will unnecessarily remount on every autosave interval, causing loss of input focus, etc. This makes it nearly impossible to edit these documents, especially if the interval is very short. But the same is true for non-autosave documents when "manually" saving, e.g. pressing the "save draft" or "publish changes" buttons. This has gone largely unnoticed, however, as the user has already lost focus of the form to interact with these controls, and they somewhat expect this behavior or at least accept it. Now, the form remains mounted across autosave events and the user's cursor never loses focus. Much better. Before: https://github.com/user-attachments/assets/a159cdc0-21e8-45f6-a14d-6256e53bc3df After: https://github.com/user-attachments/assets/cd697439-1cd3-4033-8330-a5642f7810e8 Related: #12842 --- - To see the specific tasks where the Asana app for GitHub is being used, see below: - https://app.asana.com/0/0/1210689077645986
1 parent a9580e0 commit c80b6e9

File tree

8 files changed

+162
-58
lines changed

8 files changed

+162
-58
lines changed

packages/ui/src/elements/DocumentDrawer/DrawerContent.tsx

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -42,14 +42,16 @@ export const DocumentDrawerContent: React.FC<DocumentDrawerProps> = ({
4242

4343
const [DocumentView, setDocumentView] = useState<React.ReactNode>(undefined)
4444
const [isLoading, setIsLoading] = useState(true)
45-
const hasRenderedDocument = useRef(false)
45+
const hasInitialized = useRef(false)
4646

4747
const getDocumentView = useCallback(
48-
(docID?: number | string) => {
48+
(docID?: number | string, showLoadingIndicator: boolean = false) => {
4949
const controller = handleAbortRef(abortGetDocumentViewRef)
5050

5151
const fetchDocumentView = async () => {
52-
setIsLoading(true)
52+
if (showLoadingIndicator) {
53+
setIsLoading(true)
54+
}
5355

5456
try {
5557
const result = await renderDocument({
@@ -141,13 +143,13 @@ export const DocumentDrawerContent: React.FC<DocumentDrawerProps> = ({
141143
)
142144

143145
const clearDoc = useCallback(() => {
144-
getDocumentView()
146+
getDocumentView(undefined, true)
145147
}, [getDocumentView])
146148

147149
useEffect(() => {
148-
if (!DocumentView && !hasRenderedDocument.current) {
149-
getDocumentView(existingDocID)
150-
hasRenderedDocument.current = true
150+
if (!DocumentView && !hasInitialized.current) {
151+
getDocumentView(existingDocID, true)
152+
hasInitialized.current = true
151153
}
152154
}, [DocumentView, getDocumentView, existingDocID])
153155

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
import { useCallback, useEffect, useRef, useState } from 'react'
2+
3+
/**
4+
* A hook for managing state that can be controlled by props but also overridden locally.
5+
* Props always take precedence if they change, but local state can override them temporarily.
6+
*/
7+
export function useControllableState<T>(
8+
propValue: T,
9+
defaultValue?: T,
10+
): [T, (value: ((prev: T) => T) | T) => void] {
11+
const [localValue, setLocalValue] = useState<T>(propValue ?? defaultValue)
12+
const initialRenderRef = useRef(true)
13+
14+
useEffect(() => {
15+
if (initialRenderRef.current) {
16+
initialRenderRef.current = false
17+
return
18+
}
19+
20+
setLocalValue(propValue)
21+
}, [propValue])
22+
23+
const setValue = useCallback((value: ((prev: T) => T) | T) => {
24+
setLocalValue(value)
25+
}, [])
26+
27+
return [localValue, setValue]
28+
}

packages/ui/src/providers/DocumentInfo/index.tsx

Lines changed: 24 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,10 @@
11
'use client'
2-
import type { ClientUser, DocumentPreferences, SanitizedDocumentPermissions } from 'payload'
2+
import type { ClientUser, DocumentPreferences } from 'payload'
33

44
import * as qs from 'qs-esm'
55
import React, { createContext, use, useCallback, useEffect, useMemo, useRef, useState } from 'react'
66

7+
import { useControllableState } from '../../hooks/useControllableState.js'
78
import { useAuth } from '../../providers/Auth/index.js'
89
import { requests } from '../../utilities/api.js'
910
import { formatDocTitle } from '../../utilities/formatDocTitle/index.js'
@@ -45,12 +46,11 @@ const DocumentInfo: React.FC<
4546
versionCount: versionCountFromProps,
4647
} = props
4748

48-
const [docPermissions, setDocPermissions] =
49-
useState<SanitizedDocumentPermissions>(docPermissionsFromProps)
49+
const [docPermissions, setDocPermissions] = useControllableState(docPermissionsFromProps)
5050

51-
const [hasSavePermission, setHasSavePermission] = useState<boolean>(hasSavePermissionFromProps)
51+
const [hasSavePermission, setHasSavePermission] = useControllableState(hasSavePermissionFromProps)
5252

53-
const [hasPublishPermission, setHasPublishPermission] = useState<boolean>(
53+
const [hasPublishPermission, setHasPublishPermission] = useControllableState(
5454
hasPublishPermissionFromProps,
5555
)
5656

@@ -101,15 +101,24 @@ const DocumentInfo: React.FC<
101101
unpublishedVersionCountFromProps,
102102
)
103103

104-
const [documentIsLocked, setDocumentIsLocked] = useState<boolean | undefined>(isLockedFromProps)
105-
const [currentEditor, setCurrentEditor] = useState<ClientUser | null>(currentEditorFromProps)
106-
const [lastUpdateTime, setLastUpdateTime] = useState<number>(lastUpdateTimeFromProps)
107-
const [savedDocumentData, setSavedDocumentData] = useState(initialData)
108-
const [uploadStatus, setUploadStatus] = useState<'failed' | 'idle' | 'uploading'>('idle')
104+
const [documentIsLocked, setDocumentIsLocked] = useControllableState<boolean | undefined>(
105+
isLockedFromProps,
106+
)
107+
const [currentEditor, setCurrentEditor] = useControllableState<ClientUser | null>(
108+
currentEditorFromProps,
109+
)
110+
const [lastUpdateTime, setLastUpdateTime] = useControllableState<number>(lastUpdateTimeFromProps)
111+
const [savedDocumentData, setSavedDocumentData] = useControllableState(initialData)
112+
const [uploadStatus, setUploadStatus] = useControllableState<'failed' | 'idle' | 'uploading'>(
113+
'idle',
114+
)
109115

110-
const updateUploadStatus = useCallback((status: 'failed' | 'idle' | 'uploading') => {
111-
setUploadStatus(status)
112-
}, [])
116+
const updateUploadStatus = useCallback(
117+
(status: 'failed' | 'idle' | 'uploading') => {
118+
setUploadStatus(status)
119+
},
120+
[setUploadStatus],
121+
)
113122

114123
const { getPreference, setPreference } = usePreferences()
115124
const { code: locale } = useLocale()
@@ -170,7 +179,7 @@ const DocumentInfo: React.FC<
170179
console.error('Failed to unlock the document', error)
171180
}
172181
},
173-
[serverURL, api, globalSlug],
182+
[serverURL, api, globalSlug, setDocumentIsLocked],
174183
)
175184

176185
const updateDocumentEditor = useCallback(
@@ -279,7 +288,7 @@ const DocumentInfo: React.FC<
279288
(json) => {
280289
setSavedDocumentData(json)
281290
},
282-
[],
291+
[setSavedDocumentData],
283292
)
284293

285294
/**

test/access-control/e2e.spec.ts

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -436,10 +436,15 @@ describe('Access Control', () => {
436436
const documentDrawer = page.locator(`[id^=doc-drawer_${createNotUpdateCollectionSlug}_1_]`)
437437
await expect(documentDrawer).toBeVisible()
438438
await expect(documentDrawer.locator('#action-save')).toBeVisible()
439+
439440
await documentDrawer.locator('#field-name').fill('name')
440441
await expect(documentDrawer.locator('#field-name')).toHaveValue('name')
441-
await documentDrawer.locator('#action-save').click()
442-
await expect(page.locator('.payload-toast-container')).toContainText('successfully')
442+
443+
await saveDocAndAssert(
444+
page,
445+
`[id^=doc-drawer_${createNotUpdateCollectionSlug}_1_] #action-save`,
446+
)
447+
443448
await expect(documentDrawer.locator('#action-save')).toBeHidden()
444449
await expect(documentDrawer.locator('#field-name')).toBeDisabled()
445450
})

test/access-control/payload-types.ts

Lines changed: 42 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,6 @@ export interface Config {
9595
'auth-collection': AuthCollection;
9696
'payload-locked-documents': PayloadLockedDocument;
9797
'payload-preferences': PayloadPreference;
98-
'payload-sessions': PayloadSession;
9998
'payload-migrations': PayloadMigration;
10099
};
101100
collectionsJoins: {};
@@ -126,7 +125,6 @@ export interface Config {
126125
'auth-collection': AuthCollectionSelect<false> | AuthCollectionSelect<true>;
127126
'payload-locked-documents': PayloadLockedDocumentsSelect<false> | PayloadLockedDocumentsSelect<true>;
128127
'payload-preferences': PayloadPreferencesSelect<false> | PayloadPreferencesSelect<true>;
129-
'payload-sessions': PayloadSessionsSelect<false> | PayloadSessionsSelect<true>;
130128
'payload-migrations': PayloadMigrationsSelect<false> | PayloadMigrationsSelect<true>;
131129
};
132130
db: {
@@ -232,6 +230,13 @@ export interface User {
232230
hash?: string | null;
233231
loginAttempts?: number | null;
234232
lockUntil?: string | null;
233+
sessions?:
234+
| {
235+
id: string;
236+
createdAt?: string | null;
237+
expiresAt: string;
238+
}[]
239+
| null;
235240
password?: string | null;
236241
}
237242
/**
@@ -249,6 +254,13 @@ export interface PublicUser {
249254
hash?: string | null;
250255
loginAttempts?: number | null;
251256
lockUntil?: string | null;
257+
sessions?:
258+
| {
259+
id: string;
260+
createdAt?: string | null;
261+
expiresAt: string;
262+
}[]
263+
| null;
252264
password?: string | null;
253265
}
254266
/**
@@ -740,6 +752,13 @@ export interface AuthCollection {
740752
_verificationToken?: string | null;
741753
loginAttempts?: number | null;
742754
lockUntil?: string | null;
755+
sessions?:
756+
| {
757+
id: string;
758+
createdAt?: string | null;
759+
expiresAt: string;
760+
}[]
761+
| null;
743762
}
744763
/**
745764
* This interface was referenced by `Config`'s JSON-Schema
@@ -893,26 +912,6 @@ export interface PayloadPreference {
893912
updatedAt: string;
894913
createdAt: string;
895914
}
896-
/**
897-
* This interface was referenced by `Config`'s JSON-Schema
898-
* via the `definition` "payload-sessions".
899-
*/
900-
export interface PayloadSession {
901-
id: string;
902-
session: string;
903-
expiration: string;
904-
user:
905-
| {
906-
relationTo: 'users';
907-
value: string | User;
908-
}
909-
| {
910-
relationTo: 'public-users';
911-
value: string | PublicUser;
912-
};
913-
updatedAt: string;
914-
createdAt: string;
915-
}
916915
/**
917916
* This interface was referenced by `Config`'s JSON-Schema
918917
* via the `definition` "payload-migrations".
@@ -939,6 +938,13 @@ export interface UsersSelect<T extends boolean = true> {
939938
hash?: T;
940939
loginAttempts?: T;
941940
lockUntil?: T;
941+
sessions?:
942+
| T
943+
| {
944+
id?: T;
945+
createdAt?: T;
946+
expiresAt?: T;
947+
};
942948
}
943949
/**
944950
* This interface was referenced by `Config`'s JSON-Schema
@@ -954,6 +960,13 @@ export interface PublicUsersSelect<T extends boolean = true> {
954960
hash?: T;
955961
loginAttempts?: T;
956962
lockUntil?: T;
963+
sessions?:
964+
| T
965+
| {
966+
id?: T;
967+
createdAt?: T;
968+
expiresAt?: T;
969+
};
957970
}
958971
/**
959972
* This interface was referenced by `Config`'s JSON-Schema
@@ -1294,6 +1307,13 @@ export interface AuthCollectionSelect<T extends boolean = true> {
12941307
_verificationToken?: T;
12951308
loginAttempts?: T;
12961309
lockUntil?: T;
1310+
sessions?:
1311+
| T
1312+
| {
1313+
id?: T;
1314+
createdAt?: T;
1315+
expiresAt?: T;
1316+
};
12971317
}
12981318
/**
12991319
* This interface was referenced by `Config`'s JSON-Schema
@@ -1317,17 +1337,6 @@ export interface PayloadPreferencesSelect<T extends boolean = true> {
13171337
updatedAt?: T;
13181338
createdAt?: T;
13191339
}
1320-
/**
1321-
* This interface was referenced by `Config`'s JSON-Schema
1322-
* via the `definition` "payload-sessions_select".
1323-
*/
1324-
export interface PayloadSessionsSelect<T extends boolean = true> {
1325-
session?: T;
1326-
expiration?: T;
1327-
user?: T;
1328-
updatedAt?: T;
1329-
createdAt?: T;
1330-
}
13311340
/**
13321341
* This interface was referenced by `Config`'s JSON-Schema
13331342
* via the `definition` "payload-migrations_select".

test/admin/e2e/document-view/e2e.spec.ts

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -363,6 +363,43 @@ describe('Document View', () => {
363363
})
364364

365365
describe('drawers', () => {
366+
test('document drawers do not unmount across save events', async () => {
367+
// Navigate to a post document
368+
await navigateToDoc(page, postsUrl)
369+
370+
// Open the relationship drawer
371+
await page
372+
.locator('.field-type.relationship .relationship--single-value__drawer-toggler')
373+
.click()
374+
375+
const drawer = page.locator('[id^=doc-drawer_posts_1_]')
376+
const drawerEditView = drawer.locator('.drawer__content .collection-edit')
377+
await expect(drawerEditView).toBeVisible()
378+
379+
const drawerTitleField = drawerEditView.locator('#field-title')
380+
const testTitle = 'Test Title for Persistence'
381+
await drawerTitleField.fill(testTitle)
382+
await expect(drawerTitleField).toHaveValue(testTitle)
383+
384+
await drawerEditView.evaluate((el) => {
385+
el.setAttribute('data-test-instance', 'This is a test')
386+
})
387+
388+
await expect(drawerEditView).toHaveAttribute('data-test-instance', 'This is a test')
389+
390+
await saveDocAndAssert(page, '[id^=doc-drawer_posts_1_] .drawer__content #action-save')
391+
392+
await expect(drawerEditView).toBeVisible()
393+
await expect(drawerTitleField).toHaveValue(testTitle)
394+
395+
// Verify the element instance hasn't changed (i.e., it wasn't re-mounted and discarded the custom attribute)
396+
await expect
397+
.poll(async () => {
398+
return await drawerEditView.getAttribute('data-test-instance')
399+
})
400+
.toBe('This is a test')
401+
})
402+
366403
test('document drawers are visually stacking', async () => {
367404
await navigateToDoc(page, postsUrl)
368405
await page.locator('#field-title').fill(title)

test/admin/payload-types.ts

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -293,6 +293,13 @@ export interface User {
293293
hash?: string | null;
294294
loginAttempts?: number | null;
295295
lockUntil?: string | null;
296+
sessions?:
297+
| {
298+
id: string;
299+
createdAt?: string | null;
300+
expiresAt: string;
301+
}[]
302+
| null;
296303
password?: string | null;
297304
}
298305
/**
@@ -820,6 +827,13 @@ export interface UsersSelect<T extends boolean = true> {
820827
hash?: T;
821828
loginAttempts?: T;
822829
lockUntil?: T;
830+
sessions?:
831+
| T
832+
| {
833+
id?: T;
834+
createdAt?: T;
835+
expiresAt?: T;
836+
};
823837
}
824838
/**
825839
* This interface was referenced by `Config`'s JSON-Schema

test/helpers.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -248,7 +248,7 @@ export async function saveDocHotkeyAndAssert(page: Page): Promise<void> {
248248

249249
export async function saveDocAndAssert(
250250
page: Page,
251-
selector = '#action-save',
251+
selector: '#access-save' | '#action-publish' | '#action-save-draft' | string = '#action-save',
252252
expectation: 'error' | 'success' = 'success',
253253
): Promise<void> {
254254
await wait(500) // TODO: Fix this

0 commit comments

Comments
 (0)