diff --git a/CODE-AUDIT.md b/CODE-AUDIT.md index 70b8315..6257598 100644 --- a/CODE-AUDIT.md +++ b/CODE-AUDIT.md @@ -702,8 +702,11 @@ token system + shared primitives (UI/SIMP — high value, larger effort) · i18n - [ ] **L93 — SettingsView bypasses the store layer.** It calls `window.api` directly for cookies/ffmpeg/yt-dlp/app-update/backup while using stores for settings/templates/errorlog — mixed data-access in one component. -- [ ] **L94 — `summarizeQueue` recomputed every render** in [DownloadsView.tsx](src/renderer/src/components/DownloadsView.tsx) - (no `useMemo`) — and again on every store change in App's taskbar effect. Memoize/share. +- [x] **L94 — `summarizeQueue` recomputed every render** in [DownloadsView.tsx](src/renderer/src/components/DownloadsView.tsx) + (no `useMemo`) — and again on every store change in App's taskbar effect. Memoize/share. *Fixed: a 1-entry + items-ref memo `queueSummaryOf` in [queueStats.ts](src/renderer/src/store/queueStats.ts) — the store hands + out a new items array per change, so both App's taskbar subscription and DownloadsView's render summarize + it once per change (the second caller hits the cache) instead of twice (also closes PERF4).* - [ ] **L95 — "Sign in" hyphenation varies** in the Cookies card ("Sign in…", "Site to sign in to", "Sign-in window", "Signing in…"). - [ ] **L96 — Ellipsis-on-dialog-buttons inconsistent.** "Export backup…/Import backup…/Sign in…" use `…` (opens a dialog), but "Browse" / "Check for updates" / "Update now" don't. Adopt the `…` = opens-a-dialog convention. @@ -859,8 +862,10 @@ token system + shared primitives (UI/SIMP — high value, larger effort) · i18n double-scroll or misalign the virtualized list's viewport. - [ ] **L162 — Per-thumbnail store subscription.** Every `MediaThumb` calls `useResolvedDark()` (two store subscriptions) instead of receiving a resolved theme prop — N subscriptions per list. -- [ ] **L163 — QueueItem makes 9 separate `useDownloads` selector calls** instead of one destructured - selection — repeated boilerplate per row. +- [x] **L163 — QueueItem makes 9 separate `useDownloads` selector calls** instead of one destructured + selection — repeated boilerplate per row. *Fixed: one `useShallow` selection replaces the 10 individual + subscriptions in [QueueItem.tsx](src/renderer/src/components/QueueItem.tsx) (the actions are stable, so the + shallow compare never re-renders the row).* *Round 8 (2026-06-29) — formatting micro-inconsistencies:* @@ -1698,12 +1703,16 @@ acceptable first:** memoized `QueueItem`s don't re-render when their item is unc on every add/done/error/cancel/progress-driven change; with M33's unbounded channel enqueue (thousands of items) every event becomes O(n), and several events fire per second during active downloads. **Fix:** track running/queued counts + a pointer instead of rescanning. -- [ ] **PERF4 — `summarizeQueue` runs twice per progress tick.** Once in App's store subscription (which +- [x] **PERF4 — `summarizeQueue` runs twice per progress tick.** Once in App's store subscription (which also pushes a taskbar IPC every tick, L25) and once in DownloadsView's render (no `useMemo`, L94) — each O(items), every ~second per active download. **Fix:** compute once, memoize, throttle the taskbar push. -- [ ] **PERF5 — `VirtualList` gets new function identities each render.** `estimateSize`/`getKey`/ + *Fixed with L94: `queueSummaryOf` computes the aggregate once per items change (shared ref cache), so + App's subscription and DownloadsView's render no longer each run it; the taskbar IPC push was already + deduped by computed value (L25).* +- [x] **PERF5 — `VirtualList` gets new function identities each render.** `estimateSize`/`getKey`/ `renderItem` are passed as inline arrows, so the virtualizer can't rely on stable references (risking - re-measures/cache churn). **Fix:** `useCallback`/hoist them. + re-measures/cache churn). **Fix:** `useCallback`/hoist them. *Fixed: the three callbacks are hoisted to + stable module-level functions in [DownloadsView.tsx](src/renderer/src/components/DownloadsView.tsx).* - [ ] **PERF6 — Per-thumbnail store subscriptions.** Each `MediaThumb` calls `useResolvedDark()` (two store subscriptions) (L162); a long list creates N×2 subscriptions. **Fix:** resolve once, pass as a prop. - [x] **PERF7 — (ref R3) O(n²) media-items rewrites** dominate the main-process cost when downloading a diff --git a/src/renderer/src/App.tsx b/src/renderer/src/App.tsx index 49de5a5..f15a646 100644 --- a/src/renderer/src/App.tsx +++ b/src/renderer/src/App.tsx @@ -19,7 +19,7 @@ import { useDownloads } from './store/downloads' // was guaranteed by downloads.ts importing it; now that the cycle is broken, App // owns the eager load so it survives the views becoming lazy-loaded (PERF8). import './store/sources' -import { summarizeQueue } from './store/queueStats' +import { queueSummaryOf } from './store/queueStats' import { useResolvedDark } from './store/systemTheme' import { logError } from './reportError' @@ -82,7 +82,7 @@ function App(): React.JSX.Element { let lastMode = '' let lastBadge = -1 function push(items: ReturnType['items']): void { - const s = summarizeQueue(items) + const s = queueSummaryOf(items) const mode = s.active ? (s.failed > 0 ? 'error' : 'normal') : 'none' if (s.progress === lastFraction && mode === lastMode && s.downloading === lastBadge) return lastFraction = s.progress diff --git a/src/renderer/src/components/DownloadsView.tsx b/src/renderer/src/components/DownloadsView.tsx index 8f4d905..3bed9f3 100644 --- a/src/renderer/src/components/DownloadsView.tsx +++ b/src/renderer/src/components/DownloadsView.tsx @@ -10,13 +10,19 @@ shorthands } from '@fluentui/react-components' import { ArrowDownloadRegular, ArrowClockwiseRegular } from '@fluentui/react-icons' -import { useDownloads } from '../store/downloads' -import { summarizeQueue } from '../store/queueStats' +import { useDownloads, type DownloadItem } from '../store/downloads' +import { queueSummaryOf } from '../store/queueStats' import { DownloadBar } from './DownloadBar' import { QueueItem } from './QueueItem' import { VirtualList } from './VirtualList' import { ScreenHeader, useScreenStyles } from './ui/Screen' +// Hoisted so the virtualizer gets stable function identities and doesn't churn its +// measurement/key cache on every render (PERF5). +const estimateRowSize = (): number => 100 +const getItemKey = (item: DownloadItem): string => item.id +const renderQueueRow = (item: DownloadItem): React.JSX.Element => + const useStyles = makeStyles({ root: { // Fill the scroll area so the queue list below can flex to the remaining @@ -77,7 +83,7 @@ export function DownloadsView(): React.JSX.Element { const clearFinished = useDownloads((s) => s.clearFinished) const retryAll = useDownloads((s) => s.retryAll) - const summary = summarizeQueue(items) + const summary = queueSummaryOf(items) const hasFinished = items.some( (i) => i.status === 'completed' || i.status === 'error' || i.status === 'canceled' ) @@ -146,9 +152,9 @@ export function DownloadsView(): React.JSX.Element { className={styles.listScroll} gap={10} overscan={6} - estimateSize={() => 100} - getKey={(item) => item.id} - renderItem={(item) => } + estimateSize={estimateRowSize} + getKey={getItemKey} + renderItem={renderQueueRow} /> )} diff --git a/src/renderer/src/components/QueueItem.tsx b/src/renderer/src/components/QueueItem.tsx index 77d9a75..31024a6 100644 --- a/src/renderer/src/components/QueueItem.tsx +++ b/src/renderer/src/components/QueueItem.tsx @@ -25,6 +25,7 @@ import { ErrorCircleFilled, EyeOffRegular } from '@fluentui/react-icons' +import { useShallow } from 'zustand/react/shallow' import { useDownloads, type DownloadItem } from '../store/downloads' import { thumbUrl } from '../thumb' import { fmtSchedule } from '../datetime' @@ -115,16 +116,33 @@ export const QueueItem = memo(function QueueItem({ }): React.JSX.Element { const styles = useStyles() const focus = useFocusStyles() - const cancel = useDownloads((s) => s.cancel) - const pause = useDownloads((s) => s.pause) - const resume = useDownloads((s) => s.resume) - const prioritize = useDownloads((s) => s.prioritize) - const saveForLater = useDownloads((s) => s.saveForLater) - const queueNow = useDownloads((s) => s.queueNow) - const remove = useDownloads((s) => s.remove) - const retry = useDownloads((s) => s.retry) - const openFile = useDownloads((s) => s.openFile) - const showInFolder = useDownloads((s) => s.showInFolder) + // One shallow-compared selection instead of 10 separate subscriptions per row + // (L163). The actions are stable, so this never re-renders from the store. + const { + cancel, + pause, + resume, + prioritize, + saveForLater, + queueNow, + remove, + retry, + openFile, + showInFolder + } = useDownloads( + useShallow((s) => ({ + cancel: s.cancel, + pause: s.pause, + resume: s.resume, + prioritize: s.prioritize, + saveForLater: s.saveForLater, + queueNow: s.queueNow, + remove: s.remove, + retry: s.retry, + openFile: s.openFile, + showInFolder: s.showInFolder + })) + ) const active = item.status === 'downloading' || item.status === 'queued' diff --git a/src/renderer/src/store/queueStats.ts b/src/renderer/src/store/queueStats.ts index ef0d2ed..66c79ac 100644 --- a/src/renderer/src/store/queueStats.ts +++ b/src/renderer/src/store/queueStats.ts @@ -66,6 +66,22 @@ export function summarizeQueue(items: DownloadItem[]): QueueSummary { } } +// A 1-entry memo of the last summary, keyed on the `items` array reference. The +// store hands out a new `items` array on every change, and both App's taskbar +// subscription and DownloadsView's render summarize that same array per change — so +// caching by reference computes the O(n) aggregate once per change instead of twice +// (PERF4 / L94). `summarizeQueue` stays pure for the unit tests. +let cachedItems: DownloadItem[] | null = null +let cachedSummary: QueueSummary | null = null + +/** Memoized `summarizeQueue`: recomputes only when the `items` reference changes. */ +export function queueSummaryOf(items: DownloadItem[]): QueueSummary { + if (items === cachedItems && cachedSummary) return cachedSummary + cachedItems = items + cachedSummary = summarizeQueue(items) + return cachedSummary +} + // --- Duplicate detection ---------------------------------------------------- function normalizeUrl(url: string): string {