improvement(files): react-doctor performance pass on the files module (#5330)

* improvement(files): react-doctor performance pass on the files module

* fix(files): use spread-sort not toSorted in client (Safari<16/iOS15 crash)

SWC does not polyfill Array.prototype.toSorted and the repo sets no
core-js/browserslist target, so it throws on Safari <16 / iOS 15. Revert
the two client-side toSorted calls to [...arr].sort() and correct the
harness guidance in sim-components.md accordingly.
This commit is contained in:
Waleed
2026-07-01 13:15:17 -07:00
committed by GitHub
parent 671535469d
commit 76537c890c
3 changed files with 51 additions and 29 deletions
+18
View File
@@ -12,3 +12,21 @@ Component authoring rules — structure order (refs → external hooks → store
- `'use client'` only when using React hooks or browser-only APIs.
- Prefer semantic HTML (`aside`, `nav`, `article`).
- Optional-chain callbacks: `onAction?.(id)`.
## List-render performance
When rendering or sorting a list of rows against a lookup collection (members, folders, tags), keep the per-row work O(1):
- **Precompute a lookup `Map` once**, never `array.find(...)` per row. Build `const byId = useMemo(() => { const m = new Map<string, T>(); for (const x of items ?? []) m.set(x.id, x); return m }, [items])` and read `byId.get(id)` in the sort comparator, `.map(...)`, and cell builders. A `.find` inside a sort comparator is O(n²·log n) — the worst offender. Depend memos on the derived `Map`, not the raw array.
- **Sort with `[...array].sort(cmp)` in client code — NOT `array.toSorted(cmp)`.** SWC (Next's compiler) transforms syntax but does not polyfill prototype methods, and the repo sets no core-js/browserslist target, so `toSorted`/`toReversed`/`toSpliced`/`Array.prototype.with` ship as-is and throw `TypeError` on Safari <16 / iOS 15 (they landed in Safari 16). `tsconfig` `lib: ES2023` only affects type-checking, never the runtime. The `[...arr].sort()` spread copy is the intended cost — it keeps the sort non-mutating without an unpolyfilled builtin. These methods are only safe in server-only modules (no `'use client'`, executed on Node ≥20). react-doctor's `js-tosorted-immutable` is therefore a won't-fix in client components.
- **Partition in a single pass** — when splitting one collection into several (`fileIds`/`folderIds`), do one `for…of` pushing into each bucket and return `{ a, b }` from a single `useMemo`, not two memos that each `map→filter→map` the same source twice.
## react-doctor (`npx react-doctor`) — apply the wins, skip the false positives
react-doctor diagnostics are hypotheses, not verdicts — confirm against the code before acting, and preserve behavior. Known repo-specific false positives to NOT "fix":
- `no-barrel-import` — barrel imports are the repo convention (see sim-imports.md, "Barrel Exports"). Keep them.
- `js-tosorted-immutable` — in `'use client'` code, keep `[...arr].sort(cmp)`; `toSorted` is unpolyfilled and crashes Safari <16 / iOS 15 (see "List-render performance" above). Only apply it in server-only modules.
- `rerender-state-only-in-handlers` / "state set but never rendered" — a false positive when the `useState` is consumed by a `useEffect`/`useLayoutEffect` dependency (the effect must re-run on change). Only convert to a ref when nothing reads the value reactively.
- `async-await-in-loop` on an upload/progress loop where sequential execution is intentional (per-item progress, server backpressure) — leave it.
- Broad refactors (`prefer-useReducer` for many `useState`, `no-giant-component` splits) — out of scope for a perf pass; note, don't churn.
@@ -29,15 +29,20 @@ const OwnerAvatar = memo(function OwnerAvatar({ name, image }: OwnerAvatarProps)
/**
* Resolves a user ID into a ResourceCell with an avatar icon and display name.
* Returns null label while members are still loading to avoid flashing raw IDs.
*
* Accepts either the raw member array or a precomputed `userId → member` map.
* Prefer the map form when resolving many rows so lookups stay O(1) instead of
* scanning the array per row.
*/
export function ownerCell(
userId: string | null | undefined,
members?: WorkspaceMember[]
members?: WorkspaceMember[] | Map<string, WorkspaceMember>
): ResourceCell {
if (!userId) return { label: null }
if (!members) return { label: null }
const member = members.find((m) => m.userId === userId)
const member =
members instanceof Map ? members.get(userId) : members.find((m) => m.userId === userId)
if (!member) return { label: null }
return {
@@ -78,7 +78,7 @@ import type { MoveOptionNode } from '@/app/workspace/[workspaceId]/files/move-op
import { filesParsers, filesUrlKeys } from '@/app/workspace/[workspaceId]/files/search-params'
import { useUserPermissionsContext } from '@/app/workspace/[workspaceId]/providers/workspace-permissions-provider'
import { useContextMenu } from '@/app/workspace/[workspaceId]/w/components/sidebar/hooks'
import { useWorkspaceMembersQuery } from '@/hooks/queries/workspace'
import { useWorkspaceMembersQuery, type WorkspaceMember } from '@/hooks/queries/workspace'
import {
useBulkArchiveWorkspaceFileItems,
useCreateWorkspaceFileFolder,
@@ -197,6 +197,11 @@ export function Files() {
const { data: files = EMPTY_WORKSPACE_FILES, isLoading, error } = useWorkspaceFiles(workspaceId)
const { data: folders = EMPTY_WORKSPACE_FILE_FOLDERS } = useWorkspaceFileFolders(workspaceId)
const { data: members } = useWorkspaceMembersQuery(workspaceId)
const membersById = useMemo(() => {
const map = new Map<string, WorkspaceMember>()
for (const member of members ?? []) map.set(member.userId, member)
return map
}, [members])
const uploadFile = useUploadWorkspaceFile()
const notifyLimit = useLimitUpgradeToast()
const deleteFile = useDeleteWorkspaceFile()
@@ -416,8 +421,8 @@ export function Files() {
cmp = new Date(a.updatedAt).getTime() - new Date(b.updatedAt).getTime()
break
case 'owner':
cmp = (members?.find((m) => m.userId === a.uploadedBy)?.name ?? '').localeCompare(
members?.find((m) => m.userId === b.uploadedBy)?.name ?? ''
cmp = (membersById.get(a.uploadedBy)?.name ?? '').localeCompare(
membersById.get(b.uploadedBy)?.name ?? ''
)
break
}
@@ -431,7 +436,7 @@ export function Files() {
sizeFilter,
uploadedByFilter,
activeSort,
members,
membersById,
])
const baseRows: ResourceRow[] = useMemo(() => {
@@ -453,7 +458,7 @@ export function Files() {
label: 'Folder',
},
created: timeCell(folder.createdAt),
owner: ownerCell(folder.userId, members),
owner: ownerCell(folder.userId, membersById),
updated: timeCell(folder.updatedAt),
},
}))
@@ -475,7 +480,7 @@ export function Files() {
label: formatFileType(file.type, file.name),
},
created: timeCell(file.uploadedAt),
owner: ownerCell(file.uploadedBy, members),
owner: ownerCell(file.uploadedBy, membersById),
updated: timeCell(file.updatedAt),
},
}
@@ -483,7 +488,7 @@ export function Files() {
})
return [...folderRows, ...fileRows]
}, [visibleFolders, filteredFiles, members, folderSizeMap])
}, [visibleFolders, filteredFiles, membersById, folderSizeMap])
const rows: ResourceRow[] = useMemo(() => {
if (!listRename.editingId) return baseRows
@@ -525,22 +530,16 @@ export function Files() {
const isAllSelected =
visibleRowIds.length > 0 && visibleRowIds.every((id) => selectedRowIds.has(id))
const selectedFileIds = useMemo(
() =>
Array.from(selectedRowIds)
.map(parseRowId)
.filter((item) => item.kind === 'file')
.map((item) => item.id),
[selectedRowIds]
)
const selectedFolderIds = useMemo(
() =>
Array.from(selectedRowIds)
.map(parseRowId)
.filter((item) => item.kind === 'folder')
.map((item) => item.id),
[selectedRowIds]
)
const { selectedFileIds, selectedFolderIds } = useMemo(() => {
const fileIds: string[] = []
const folderIds: string[] = []
for (const rowId of selectedRowIds) {
const item = parseRowId(rowId)
if (item.kind === 'file') fileIds.push(item.id)
else folderIds.push(item.id)
}
return { selectedFileIds: fileIds, selectedFolderIds: folderIds }
}, [selectedRowIds])
const selectableConfig = useMemo(
() => ({
@@ -1749,7 +1748,7 @@ export function Files() {
uploadedByFilter.length === 0
? 'All'
: uploadedByFilter.length === 1
? (members?.find((m) => m.userId === uploadedByFilter[0])?.name ?? '1 member')
? (membersById.get(uploadedByFilter[0])?.name ?? '1 member')
: `${uploadedByFilter.length} members`
return (
@@ -1831,7 +1830,7 @@ export function Files() {
)}
</div>
)
}, [typeFilter, sizeFilter, uploadedByFilter, memberOptions, members, hasActiveFilters])
}, [typeFilter, sizeFilter, uploadedByFilter, memberOptions, membersById, hasActiveFilters])
const filterTags: FilterTag[] = useMemo(() => {
const tags: FilterTag[] = []
@@ -1863,12 +1862,12 @@ export function Files() {
if (uploadedByFilter.length > 0) {
const label =
uploadedByFilter.length === 1
? `Uploaded by: ${members?.find((m) => m.userId === uploadedByFilter[0])?.name ?? '1 member'}`
? `Uploaded by: ${membersById.get(uploadedByFilter[0])?.name ?? '1 member'}`
: `Uploaded by: ${uploadedByFilter.length} members`
tags.push({ label, onRemove: () => setUploadedByFilter([]) })
}
return tags
}, [typeFilter, sizeFilter, uploadedByFilter, members])
}, [typeFilter, sizeFilter, uploadedByFilter, membersById])
if (fileIdFromRoute && !selectedFile && isLoading) {
return (