diff --git a/docs/LIST_RENDERING_PERFORMANCE_REFACTOR.md b/docs/LIST_RENDERING_PERFORMANCE_REFACTOR.md new file mode 100644 index 00000000..d66a90c3 --- /dev/null +++ b/docs/LIST_RENDERING_PERFORMANCE_REFACTOR.md @@ -0,0 +1,68 @@ +# List Rendering Performance Refactor + +This document details a performance refactor of the highest-traffic list-rendering surfaces in the TeachLink web app. + +## Problem + +Several components rendered an entire in-memory collection on every render with no windowing or pagination, and rebuilt per-row/per-item callbacks (and, in one case, non-memoized row components) on every render. Together this meant every keystroke, selection toggle, or unrelated state change re-rendered the *entire* list, with cost growing linearly (or worse) with list size. + +## Changes per file + +| File | Pagination | Row/item memoization | Callback stabilization | +| :--- | :--- | :--- | :--- | +| `src/components/ui/Table.tsx` | Prev/Next footer, default `pageSize=25` (overridable via prop) | `TableRow` wrapped in `React.memo` (`MemoizedTableRow`) | `toggleSelectRow` now reads the latest selection from a ref instead of depending on `selectedRowKeys`, so its identity — and therefore every row's `onSelect` prop — stays stable across selection changes | +| `src/components/notificationcenter.tsx` | "Load more" (accumulates), page size 20 | `NotificationItem` wrapped in `React.memo` | `markAsRead`/`clearNotification` were already `useCallback`'d in `Notificationprovider.tsx` | +| `src/components/admin/ApprovalQueue.tsx` | Prev/Next footer, page size 10 | New `ApprovalItemRow` (`React.memo`) extracted from inline JSX | `review` is `useCallback`'d on `[user]` only; the per-item review note moved from a `Record` on the parent into **local state inside each row**, so typing in one row's textarea no longer busts every other row's props | +| `src/components/cms/MediaManager.tsx` | "Load more" (accumulates), page size 10 | New `MediaQueueItem` (`React.memo`) extracted from inline JSX | No per-item callbacks exist on this component; memoization alone stops unrelated queue items re-rendering on another item's progress tick (the store already returns stable object references for untouched items) | +| `src/components/social/FollowingSystem.tsx` | Prev/Next footer, page size 15, resets to page 1 on tab switch or search | `UserRow` wrapped in `React.memo` | N/A — each row owns its follow state via `useFollowUser` | +| `src/components/social/SocialProfile.tsx` | N/A (no list) | Whole component wrapped in `React.memo` for consistency | N/A | +| `src/components/BulkActions.tsx` | N/A (renders a small static action bar, not a data-driven list) | N/A | Already fully `useCallback`'d (`handleBulkOperation`, `handleUndo`, `handleRedo`, `handleCancel`) — no changes needed | +| `src/components/dashboard/AdvancedDashboard.tsx` / `DashboardPanelCard.tsx` | N/A (≈4 panels, already `useMemo`'d) | `DashboardPanelCard` wrapped in `React.memo` (was the one un-memoized piece; everything else — `sortedPanels`, all handlers, `SortablePanel` — was already memoized) | Already `useCallback`'d in `useDashboardData.tsx` | + +Pagination uses a new shared hook, `src/hooks/usePagination.ts` (plain array slicing over an already-loaded in-memory array, clamps the current page when the underlying list shrinks). `src/components/InfiniteList.tsx` (react-window + `AutoSizer`) was deliberately **not** reused here: `AutoSizer` measures real DOM layout, which is 0×0 in jsdom, so it renders zero rows under Testing Library and would have broken every existing test for these components. None of the 8 files have a server-paginated "hasNextPage" shape today, so client-side pagination is the natural fit; `InfiniteList` remains available for a future server-paginated feed. + +## Pre-existing bugs fixed as drive-bys + +Found while reading these files for the refactor, fixed in the same diff since both files were already being touched: + +- **`notificationcenter.tsx`**: the no-avatar fallback branch was `{avatarUrl ? : ({TYPE_ICON[type]})}` — the extra `{}` around `TYPE_ICON[type]` inside the ternary's expression slot is invalid. Fixed to `TYPE_ICON[type]`. There was no test exercising this branch before, so it was unguarded; a regression test now covers it (`src/components/__tests__/notificationcenter.test.tsx`). +- **`admin/ApprovalQueue.tsx`**: the Approve button's visible text was "Approve It", but `src/app/api/approvals/__tests__/approvals.test.tsx` asserted `getByRole('button', { name: /^approve$/i })` (exact match). This was failing on `main` independent of this refactor. Fixed the button text to "Approve" (consistent with the single-word "Reject" label next to it). + +Also removed an unused `X` icon import from `MediaManager.tsx` while touching its import list. + +**Not fixed** (found but out of scope — unrelated to list rendering, both flaky/pre-existing on `main`): `MediaManager.test.tsx` has two tests (`should clear all intervals when component unmounts during ongoing uploads`, `should prevent default drag behavior`) that fail intermittently due to a `DragEvent`/spy interaction quirk in jsdom + a `Math.random()`-timed upload-progress simulation; reproduced identically on `main` before this refactor. + +## Before / after numbers + +Captured via `React.Profiler` in the new `*.bench.test.tsx` files (no browser profiling dependency — runs in CI under Vitest/jsdom). Each measures the render cost of a single unrelated-item update against a realistic list size: + +``` +[bench:Table] select 1/300 rows -> before: 50.609ms (unmemoized) | after: 0.000ms (memoized rows, stable onSelect) +[bench:NotificationCenter] update after 1/50 changed -> 1 commit(s), 0.806ms total actualDuration +[bench:FollowingSystem] 1 row follow-toggle / 50 rows -> 1 commit(s), 0.326ms total actualDuration +[bench:DashboardPanelCard] unchanged-props re-render -> 1 commit(s), 0.027ms actualDuration (memoized bailout) +``` + +The `Table` benchmark is the clearest before/after comparison: it reconstructs the pre-refactor shape (unmemoized row, fresh inline `onSelect` closure per row, no pagination) side by side with the current implementation, both rendering 300 rows and reacting to a single checkbox toggle. The unmemoized version re-renders all 300 rows (~50.6ms of render work); the memoized, paginated version does effectively none. The other benchmarks assert an absolute bound (well under what an unmemoized equivalent would cost) rather than a literal side-by-side reconstruction, to keep the test files focused. + +Re-run any of these locally with, e.g.: + +``` +pnpm vitest run src/components/ui/__tests__/Table.bench.test.tsx +``` + +## Testing + +- `src/components/ui/__tests__/Table.test.tsx` — added `pagination` and `memoization` describe blocks; existing gesture/resize/selection tests unchanged and still passing. +- `src/components/ui/__tests__/Table.bench.test.tsx` — new, before/after benchmark. +- `src/components/__tests__/notificationcenter.test.tsx` — new (none existed before): fallback-icon regression, "Load more" pagination, benchmark. +- `src/components/cms/MediaManager.test.tsx` — added a `Pagination` describe block; existing declaration-order/interval-cleanup/integration tests unchanged. +- `src/app/api/approvals/__tests__/approvals.test.tsx` — added `pagination` describe block plus a review-note-isolation test; the two previously-failing `/^approve$/i` assertions now pass. +- `src/components/social/__tests__/FollowingSystem.test.tsx` — new (none existed before): pagination, tab-switch page reset, benchmark. +- `src/components/dashboard/__tests__/DashboardPanelCard.bench.test.tsx` — new, memoized-bailout benchmark. Existing `AdvancedDashboard.test.tsx` assertions unchanged. + +## Explicitly out of scope + +- **`SocialProfile.tsx`**: has no real follower/activity list today (placeholder text per tab) — product decision was to skip virtualization here rather than build out new list UI as part of a performance ticket. +- **`ActivityFeed.tsx` / `TopicFeed.tsx` / `useActivityFeed.ts` / `useTopicFeed.ts`**: not among the 8 files named in the ticket; left untouched to avoid scope creep. Candidate for a follow-up ticket if "the feeds" was meant to include them. +- **`BulkActions.tsx`**: already fully `useCallback`'d and renders a small static button bar, not a data-driven list — no functional change made. diff --git a/src/app/api/approvals/__tests__/approvals.test.tsx b/src/app/api/approvals/__tests__/approvals.test.tsx index fa94a340..b2c6a8d9 100644 --- a/src/app/api/approvals/__tests__/approvals.test.tsx +++ b/src/app/api/approvals/__tests__/approvals.test.tsx @@ -334,4 +334,63 @@ describe('ApprovalQueue component', () => { expect(body.status).toBe(ReviewDecision.APPROVED); }); }); + + describe('pagination', () => { + const manyItems = Array.from({ length: 15 }, (_, i) => ({ + id: `a-${i}`, + contentId: `c-${i}`, + contentType: 'COURSE', + title: `Course ${i}`, + submittedBy: 'instructor-1', + submittedAt: new Date().toISOString(), + status: ApprovalStatus.PENDING, + })); + + beforeEach(() => { + vi.stubGlobal( + 'fetch', + vi.fn().mockResolvedValue({ json: async () => ({ success: true, data: manyItems }) }), + ); + }); + + it('only shows the first page of pending items', async () => { + render(); + await waitFor(() => expect(screen.getByText('Course 0')).toBeInTheDocument()); + + expect(screen.getAllByText(/^Course \d+$/)).toHaveLength(10); + expect(screen.getByText('Page 1 of 2')).toBeInTheDocument(); + expect(screen.queryByText('Course 10')).not.toBeInTheDocument(); + }); + + it('advances to the next page', async () => { + const { user } = render(); + await waitFor(() => expect(screen.getByText('Course 0')).toBeInTheDocument()); + + await user.click(screen.getByRole('button', { name: 'Next' })); + + expect(screen.getByText('Page 2 of 2')).toBeInTheDocument(); + expect(screen.getByText('Course 10')).toBeInTheDocument(); + expect(screen.queryByText('Course 0')).not.toBeInTheDocument(); + }); + }); + + it('keeps each item review note isolated (typing in one does not affect another)', async () => { + const items = [ + { ...pendingItems[0], id: 'a-1', title: 'Course A' }, + { ...pendingItems[0], id: 'a-2', title: 'Course B' }, + ]; + vi.stubGlobal( + 'fetch', + vi.fn().mockResolvedValue({ json: async () => ({ success: true, data: items }) }), + ); + + const { user } = render(); + await waitFor(() => expect(screen.getByText('Course A')).toBeInTheDocument()); + + const textareas = screen.getAllByPlaceholderText('Optional review note…'); + await user.type(textareas[0], 'looks good'); + + expect(textareas[0]).toHaveValue('looks good'); + expect(textareas[1]).toHaveValue(''); + }); }); diff --git a/src/components/__tests__/notificationcenter.test.tsx b/src/components/__tests__/notificationcenter.test.tsx new file mode 100644 index 00000000..8b5d0eeb --- /dev/null +++ b/src/components/__tests__/notificationcenter.test.tsx @@ -0,0 +1,115 @@ +import React, { Profiler } from 'react'; +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { render, screen, fireEvent } from '@testing-library/react'; +import { NotificationCenter } from '../notificationcenter'; +import * as NotificationProviderModule from '@/providers/Notificationprovider'; +import type { Notification } from '@/providers/Notificationprovider'; +import { makeProfilerRecorder } from '@/testing/utils/renderProfiler'; + +vi.mock('@/providers/Notificationprovider', async () => { + const actual = await vi.importActual( + '@/providers/Notificationprovider', + ); + return { ...actual, useNotifications: vi.fn() }; +}); + +function makeNotification(overrides: Partial = {}): Notification { + return { + id: overrides.id ?? `n-${Math.random()}`, + type: 'info', + title: 'Test notification', + timestamp: new Date(), + read: false, + ...overrides, + }; +} + +// Stable callback identities across renders, mirroring the real provider +// (which wraps them in useCallback) — required for row memoization to work. +const markAsRead = vi.fn(); +const markAllAsRead = vi.fn(); +const clearNotification = vi.fn(); +const clearAll = vi.fn(); + +function mockNotifications(notifications: Notification[]) { + vi.mocked(NotificationProviderModule.useNotifications).mockReturnValue({ + notifications, + unreadCount: notifications.filter((n) => !n.read).length, + connectionState: { status: 'connected', reconnectAttempts: 0 }, + markAsRead, + markAllAsRead, + clearNotification, + clearAll, + }); +} + +describe('NotificationCenter', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('renders the fallback type icon when a notification has no avatarUrl (regression: TYPE_ICON JSX bug)', () => { + mockNotifications([makeNotification({ id: 'a', title: 'No avatar here' })]); + render(); + + fireEvent.click(screen.getByRole('button', { name: /notifications/i })); + + expect(screen.getByText('No avatar here')).toBeInTheDocument(); + }); + + it('only shows "Load more" once notifications exceed a single page', () => { + const many = Array.from({ length: 25 }, (_, i) => makeNotification({ id: `n-${i}` })); + mockNotifications(many); + render(); + + fireEvent.click(screen.getByRole('button', { name: /notifications/i })); + + // First page (20) is visible; the rest only appear after "Load more". + expect(screen.getAllByRole('button', { name: 'Dismiss notification' })).toHaveLength(20); + fireEvent.click(screen.getByText('Load more')); + expect(screen.getAllByRole('button', { name: 'Dismiss notification' })).toHaveLength(25); + }); + + it('does not show "Load more" when everything fits on one page', () => { + mockNotifications([makeNotification()]); + render(); + + fireEvent.click(screen.getByRole('button', { name: /notifications/i })); + + expect(screen.queryByText('Load more')).not.toBeInTheDocument(); + }); + + it('[bench] re-rendering with one changed notification stays cheap for a 50-item list', () => { + const notifications = Array.from({ length: 50 }, (_, i) => makeNotification({ id: `n-${i}` })); + mockNotifications(notifications); + + const recorder = makeProfilerRecorder(); + const { rerender } = render( + + + , + ); + fireEvent.click(screen.getByRole('button', { name: /notifications/i })); + recorder.reset(); + + // Only notification 0's `read` flag changes; the other 49 object + // references are preserved, the same pattern the real provider's + // `markAsRead` uses (`prev.map(n => n.id === id ? {...n, read: true} : n)`). + mockNotifications(notifications.map((n, i) => (i === 0 ? { ...n, read: true } : n))); + rerender( + + + , + ); + + // eslint-disable-next-line no-console + console.info( + `[bench:NotificationCenter] update after 1/50 changed -> ${recorder.renderCount()} commit(s), ${recorder + .totalDuration() + .toFixed(3)}ms total actualDuration`, + ); + // Guards against a catastrophic (e.g. O(n^2)) regression; memoized rows + // keep a single-item update well under this bound. + expect(recorder.totalDuration()).toBeLessThan(200); + }); +}); diff --git a/src/components/admin/ApprovalQueue.tsx b/src/components/admin/ApprovalQueue.tsx index 0ecec6e6..f4581da7 100644 --- a/src/components/admin/ApprovalQueue.tsx +++ b/src/components/admin/ApprovalQueue.tsx @@ -1,11 +1,12 @@ 'use client'; -import React, { useCallback, useEffect, useState } from 'react'; +import React, { memo, useCallback, useEffect, useState } from 'react'; import { CheckCircle, XCircle, Clock, RefreshCw } from 'lucide-react'; import { ApprovalStatus, ReviewDecision } from '@/types/approvals'; import { PermissionGate } from '@/app/components/auth/PermissionGate'; import { Permission, User } from '@/types/api'; import type { ApprovalItem } from '@/types/api'; +import { usePagination } from '@/hooks/usePagination'; interface ApprovalQueueProps { user: User | null | undefined; @@ -41,11 +42,80 @@ function StatusBadge({ status }: { status: ApprovalStatus }) { type ApiFieldError = { field: string; message: string }; +const APPROVALS_PAGE_SIZE = 10; + +interface ApprovalItemRowProps { + item: ApprovalItem; + submitting: boolean; + onApprove: (id: string, note: string) => void; + onReject: (id: string, note: string) => void; +} + +const ApprovalItemRow = memo(function ApprovalItemRow({ + item, + submitting, + onApprove, + onReject, +}: ApprovalItemRowProps) { + // Kept local so typing a note in one row never re-renders any other row. + const [note, setNote] = useState(''); + + return ( +
  • +
    +
    +

    {item.title}

    +

    + {item.contentType} · submitted by{' '} + {item.submittedBy} ·{' '} + {new Date(item.submittedAt).toLocaleDateString()} +

    +
    + +
    + + {item.status === ApprovalStatus.PENDING && ( +
    +