perf: engine optimization pass (memory/CPU/GPU) - #169
Conversation
background.jpg was a 4.35MB, 3840x2160 progressive JPEG rendered as a full-window CSS background (bg-cover) behind only a light 2-4px backdrop-blur, and reused as-is on the loading screen and every popup dialog panel. Recompressed losslessly-in-effect with CaesiumCLT (--quality 90, dimensions untouched): 4.56MB -> 1.58MB (-65.3%), PSNR 40.53dB against the original (>40dB is the conventional "visually lossless" threshold; self-compare sanity-checked at 0 distortion). Less to decode and raster on every paint of the window background, loading screen, and any popup.
The ModDB grid can hold hundreds of cards, and none of them were memoized: every keystroke in the filter bar (or any unrelated re-render of ListMods, e.g. editing an installation's start params from elsewhere in the app) redrew every visible card. Wrapped ModListCard in React.memo, but memo alone was a no-op here, so this also had to fix why its props kept changing identity: - ModsGrid wrapped onSelect/onToggleFav/onOpenModDb in a fresh arrow function per card on every render. ModListCard now takes the mod as an argument on those handlers and does its own binding internally, so ModsGrid can hand every card the same reference instead. - ListMods created onSelectMod/onToggleFavMod/onOpenModDb inline, a new function every render; wrapped in useCallback. - onSelectMod's useCallback depended on the `installation` object itself, which ListMods rebuilds via useMemo on every edit to *any* installation (unrelated fields included) because it's derived from the whole installations array. Only ever used for a truthiness check, so it now depends on a `hasInstallation` boolean instead. - The shared useExternalLinks hook (and its mods-feature wrapper) returned a brand-new closure on every call with no memoization at all, which alone was enough to break onOpenModDb's identity regardless of the fixes above. Wrapped both in useCallback; every other consumer of this hook benefits too. tests/renderer-dom/modsGridMemoization.test.tsx replaces ModListCard with a render-count spy and asserts it stays at 1 render across an unrelated ListMods re-render. Measured before this change (with only the test file applied): 2 renders for that scenario. After: 1.
The mods grid can hold hundreds of cards, each an <img src="cachemodimg:...">, and every single one hit the protocol handler's lstat+realpath check and a net.fetch disk read on every request, with nothing caching the actual bytes between them. Names in this cache folder are content-addressed (sha256 of the file's own bytes, see iconCache.ts), so a hit can never serve stale data: nothing can rewrite a name's content without changing the name. That's what makes a plain byte cache safe here with no invalidation logic at all, just recency-based eviction under a 16MB budget (a quarter of the on-disk 64MB one, since the grid only ever needs what's currently in view). The lstat+realpath safety check still runs on every request unconditionally; only the disk read past that point is skipped on a hit. tests/domain/mods/iconMemoryCache.test.ts covers the cache's own eviction behavior and, with a counted stub standing in for the disk read, confirms readIconWithMemoryCache reads a given icon from disk exactly once no matter how many times it's requested afterward.
Nothing capped how many DOWNLOAD_ON_PATH/EXTRACT_ON_PATH/COMPRESS_ON_PATH calls the renderer could fire at once: each one spun up its own worker thread with no ceiling, so a burst (a mod update-all, say) meant an unbounded number of concurrent worker threads. Added a ConcurrencyLimiter (a small FIFO counting semaphore) and wrapped the three worker-spawning calls: 3 concurrent downloads, and extraction/compression sharing a separate limit of 2 since both spawn a 7-Zip subprocess and compete for the same CPU cores. A queued call is just a promise that hasn't resolved yet; TaskManagerContext already renders that as "pending" until the first progress event, so this needed no renderer-side change. tests/ipc/pathsHandlers.test.ts adds an integration test firing 4 concurrent DOWNLOAD_ON_PATH calls and confirming the 4th's worker isn't created until one of the first 3 finishes.
ModListCard already uses content-visibility: auto (the skip-offscreen-render utility in styles.css) to skip layout/paint for cards scrolled out of view, but the installed-mods list in ManageMods -- which can run just as long, since it's a user's own Mods folder rather than a search result -- never got the same treatment. Its rows are a fixed h-20, so the same "no contain-intrinsic-size" reasoning already written down for the grid applies here too: the value would depend on the uiscale setting, so it's left for the browser to measure once rather than guessed. Scoped to InstalledModItem only, not ErrorInstalledModItem or the generic ListItem/GridItem primitives: those either have no reliable size per call site or are only ever used in short lists (ListInstallations, ListVersions), where there's nothing off screen worth skipping. manageMods.test.tsx asserts the class lands on a rendered row. The actual paint/layout saving itself isn't observable in jsdom -- it's the same technique already trusted for ModListCard, just applied to the one other component with the same long-list, fixed-row-height shape.
useQueryMods refetched from the network on every call even for the exact same filters (toggling a filter off and back on, or leaving the Mods page and returning, both re-ran the same request from scratch). Added a small module-scoped, TTL'd (2 min) cache keyed by the exact request path the hook already builds, capped at 20 distinct filter combinations. Separately: ListMods' triggerQueryMods had no defense against two overlapping calls (a filter changed again before the first request came back) -- whichever resolved last won, regardless of which was asked for last, so a slow, already- stale query could silently overwrite a filter the user had since moved past. A per-call token now makes a superseded query's result a no-op instead. True network-level cancellation isn't implemented: Electron's ipcRenderer.invoke has no cancellation primitive without adding a whole new IPC surface (a cancel channel, request ids tracked in main, wiring requestBoundedText to honor it), which is a bigger change than this warranted -- the abandoned request still completes in the main process, it just can no longer affect the UI. tests/renderer-dom/useQueryMods.test.tsx covers the cache (hit, miss on different filters, onFinish still firing on a hit). modsListMods.test.tsx adds the out-of-order scenario directly: a slower "old" search and a faster "new" one, asserting the newer one's result survives the older one finally resolving after it. The cache is module-scoped by design (every ListMods instance shares the same ModDB catalog), which also means it outlives a single test unless cleared -- clearQueryCache() is now called in modsListMods.test.tsx's beforeEach so one test's cached response can't leak into another's assertions.
validateArchive (archiveValidation.ts) ran on the main process's event loop, in the EXTRACT_ON_PATH IPC handler, before the extraction worker was even started. Its 7-Zip path (validateSevenZipArchive) spawns `7z l` and then parses the listing synchronously: up to 100,000 entries over as much as 4MB of text, real CPU work with no business blocking the main thread when a worker thread is about to exist for this exact archive regardless. Moved the call into runExtraction itself (workers/extraction.ts), as its first step, before the temporary/target directories are even created. It was already the first of two validation gates by design (the worker re-validates the extracted tree in its own temp folder afterward); this just moves gate one to sit where gate two already lived, both now inside the worker instead of split across the main thread and the worker. extractWorker.ts now forwards the actual rejection message instead of a generic "Extraction failed" for every failure, validation included -- the specific reasons (e.g. "Archive contains an unsafe entry") were previously being swallowed for every failure past the point that used to be gate one, which is a more informative error for a user or a bug report either way. tests/ipc/extraction.test.ts's existing "cannot be read" case now asserts against the more specific message and confirms the target directory is never even created (previously it was created empty, since validation used to run after that point). The pre-existing symlink-archive rejection test needed no changes: its regex already matched either gate's wording.
GridItem's useInView call used once: false, motion/react's default. That means the IntersectionObserver it creates per card (see node_modules/motion's render/dom/viewport/index.mjs) never gets unobserved. It stays alive for the card's whole lifetime, firing on every scroll in and out, and each firing flips isInView back to false and then true again, replaying the entrance fade every time a card re-enters the viewport, not just the first time. Switched to once: true. Once a card has been seen, motion's own inView() implementation calls observer.unobserve() on it, a real, documented optimization in the library itself, not something this change invents. The one visible behavior difference: a card that's been scrolled past once no longer re-fades in on a later re-entry, it just stays visible. Nothing else about the animation changes. Scoped to GridItem only. List.tsx's ListItem doesn't use useInView at all, so this doesn't apply there. A broader LazyMotion + `m` component migration across all 19 files using motion/react was considered too (real bundle-size win, well-documented technique) but skipped here: it's all-or-nothing across every file for the bundle savings to materialize, and this pass has no working way to visually verify a change that wide in this environment. tests/renderer-dom/gridItemInView.test.tsx installs a controllable fake IntersectionObserver (the one in setup.ts is a permanent no-op, so it can never fire a callback to test against) and runs motion's real useInView/ inView code against it: confirms unobserve() gets called once a card is first seen. Verified against the pre-fix code too: without once: true, the same test's assertion fails with zero unobserve calls.
Pixnop
left a comment
Pixnop
left a comment
There was a problem hiding this comment.
I had this one verified hard, including live before/after measurement, because a +852 perf pass is where plausible-but-wrong hides best. The good news first, and it is genuinely good: zero Electron or Chromium flag changes, so the classic cargo-cult risk is absent; the content-visibility use on installed rows is sound (fixed-height element, no clipped descendants); moving validateArchive into the worker keeps defense in depth (removing gate one still fails the symlink archive at gate two, checked by mutation); the useCallback dependency arrays are all complete today; the PSNR claim on the background reproduces to the decimal; and the icon protocol's security ordering is right, the symlink check still runs before the cache. All gates pass with the exact numbers the body states.
Three changes before merge.
First, the queueing introduces a real shutdown hazard, verified in source. ConcurrencyLimiter.run releases its slot in a finally, so when before-quit's terminateActiveWorkers rejects the in-flight tasks, each rejection frees a slot, the queue drains, and createTrackedWorker spawns brand-new workers after the sweep already iterated the set. Since before-quit can preventDefault for the config flush, there is a live window where fresh downloads and extractions start writing to disk during shutdown, a hazard that did not exist before queueing. A shutdown flag the limiter checks before dequeuing (reject instead of run once quitting began) closes it in a few lines.
Second, the flagship optimizations are unprotected, shown by mutation: deleting the memo() on ModListCard leaves all 122 renderer-dom tests green (the memoization test mocks the card and wraps the spy in its own memo, so it pins the callback chain but not the export), and removing favMods from onToggleFavMod's deps also stays green while silently breaking un-favoriting, the correct-perf-wrong-data class. The single highest-leverage fix: add eslint-plugin-react-hooks with exhaustive-deps as an error. This PR makes the hot path depend on hand-written dep arrays; nothing currently catches a wrong one. A test that pins the memo through the real export (render, re-render parent with identical props, assert the card body executed once) covers the rest.
Third, the body needs to say what the queue changes for users, because three of the four behavior changes are user-visible: queued tasks hold the app-close blocker while pending (twenty mod updates now keep the window un-closable behind one slow download), the cap covers three of the five worker spawn sites (RUN_INSTALLER and CHANGE_PERMS stay uncapped, and the body reads as if the burst problem is solved), and one global FIFO means a game-version download queues behind mod downloads with no priority. State them or narrow the claims.
On the record as follow-ups rather than blockers: the module-scope icon memory cache is never cleared, so up to 16 MB stays resident in main forever after one grid visit, a strictly negative steady-state item in a memory-titled PR, and its 16 MB budget is only honored because a different file caps icons at 512 KB, a cross-file invariant neither documented nor tested; the cachemodimg protocol integration has zero tests; clearQueryCache belongs in global test setup rather than one file's beforeEach; and the background image kept its 3840x2160 dimensions, so the decode-and-raster framing in the commit overstates it, the real win is 3 MB off the installer while a 1440p re-export is where the memory actually is. Finally, measured dev-mode RSS and idle CPU show no detectable head-vs-base delta (renderer RSS varies 60 MB between identical runs), so the memory/CPU/GPU title over-promises what the body honestly does not claim.
…st.ts FakeIntersectionObserver.instances[0] typechecks as possibly undefined under this project's tsconfig, even right after asserting the array has length 1: TypeScript does not narrow an index access from a separate length check. Missed in the original commit because I only ran typecheck:web there instead of the full typecheck (node + web + tests), and tests run under their own tsconfig.
Every download, extraction, compression, and permissions change spun up a fresh worker thread and terminated it right after, even for a burst of back-to-back tasks (installing several mods in a row downloads, then extracts, several times over). Each new Worker pays the full cost of a new V8 isolate, roughly 10 to 30MB and some milliseconds, for no reason when the same kind of work is about to run again seconds later. This is a genuine trade-off, not a pure win: keeping idle workers alive costs resident memory the old zero-idle-footprint design avoided. Sized conservatively to keep that cost bounded: at most 3 idle download workers, 2 idle archive workers (extraction and compression share one limit since both spawn a 7-Zip subprocess and compete for the same CPU cores), 1 idle compression worker on top of that shared cap, and 0 for CHANGE_PERMS and RUN_INSTALLER, which run once per install with no burst behind them and would only pay for an idle isolate with nothing to show for it. An idle worker not reused within 30 seconds is terminated. A worker is also retired after 50 tasks regardless of reuse, a bound against any per-task leak inside one isolate over a launcher session that runs for days. The reason this needed more than a small tweak: workerData is read once, at module load, and is fixed for a Worker's whole lifetime. Node has no way to hand new workerData to an already-running worker. Reuse means the five worker scripts (download, extract, compress, changeperms, innoextract) had to stop reading their task out of workerData and instead wait on parentPort for a message, run it, report the result, and go back to waiting for the next one rather than exiting. That shared protocol now lives in one place, workers/workerHost.ts's serveTasks, instead of being duplicated five times: each shim is now a handler function plus a failure describer, about eight lines each. workerPool.ts is the pool itself, independent of Electron so it stays testable with a fake worker factory. acquire() is synchronous on purpose: nothing yields between taking a worker off the idle list and marking it busy, which is what makes it impossible for two callers racing for the same script to be handed the same worker, and impossible for an idle timer to fire on a worker that was just taken. runTrackedWorker in pathsHandlers.ts now asks a worker for reuse or discards it depending on how a task actually ended, not always terminating: a clean finish or a reported task failure both leave the worker fit to serve another task (every logic module under src/ipc/workers/ cleans up its own temp state in a finally block or its own failure path), while a native worker error, an unexpected exit, or a timeout all discard it, since the worker's state, or the abandoned task still running inside it, can no longer be trusted. Messages now carry a token so a reused worker's late message from an abandoned task, arriving after a new task has already been dispatched to the same thread, gets dropped instead of being mistaken for the new task's own progress or result. Tests: workerPool.test.ts covers the pool's own bookkeeping against a fake worker factory (reuse, discard, the idle cap, idle timeout and its cancellation on reuse, a worker dying while idle, retirement after maxTasksPerWorker, per-script isolation). workerHost.test.ts covers the worker-side protocol against a fake parentPort (token echo, a stray progress report after settling, a task arriving while busy, a synchronously throwing handler). Both existing pathsHandlers test files are updated for the new acquireWorker-based mock shape and gained disposition assertions (reuse vs discard) at each settle path, plus new cases for the retire flag and a stale token being ignored. One thing no mock can prove: that a real worker thread with a "message" listener attached actually stays alive after finishing a task instead of exiting, which is the Node-level guarantee the whole design depends on. workerPoolLiveThread.test.ts checks it directly, no aliasing or workerHost import, against a real worker_threads.Worker running a plain inline script: post a task, get an answer, post a second task on the same worker, get a second answer. Verified end to end past the unit tests too: a full electron-vite build plus electron-builder package, confirming workerHost.js comes out as its own shared chunk that each of the five compiled worker entry files requires, and a run of the packaged app confirms it starts cleanly with the pool wired in from the first import.
…rkerPool The config flush in before-quit calls event.preventDefault(), so quitting is not instant. During that window a queued download, extraction, or compression could still reach the front of ConcurrencyLimiter's queue and start writing to disk, and WorkerPool.acquire() had no matching check, so it could still spawn a fresh worker thread after terminateAll() had already run. Both fixes give each object a one-way shuttingDown flag: ConcurrencyLimiter now rejects every queued task the instant shutdown() runs and rejects on arrival for anything after, wired to app's own before-quit in pathsHandlers.ts; WorkerPool.acquire() now throws instead of spawning once terminateAll() has already fired. Found in review on PR #169.
The existing modsGridMemoization.test.tsx mocks ModListCard's whole module and wraps its own mock function in memo() before asserting the mock only rendered once, which only proves memo works on a spy: dropping memo() from the real ModListCard.tsx leaves that suite green, since the mock the test actually imports never changes. Confirmed by removing memo() from the real export and watching that suite still pass. This test imports the real, unmocked ModListCard and wraps the mod object it renders in a Proxy that counts property reads. Only the component's own function body ever reads a property off mod; memo's bailout check is Object.is(prevProps.mod, nextProps.mod), comparing the Proxy reference itself without touching a property on it. So the read count only advances when ModListCard's body actually runs. A positive control test (toggling isFav for real) proves the counter can detect a re-render at all, so the main test cannot pass vacuously. Verified against a real regression: removing memo() from ModListCard.tsx fails this suite, restoring it passes again. Found in review on PR #169.
configReducer's ADD_FAV_MOD pushes onto favMods without checking for an existing entry, so onToggleFavMod only stays correct because it reads a live favMods off its own useCallback dependency array rather than a stale closure from the render that first mounted the button. Nothing exercised clicking the same mod's favorite button twice in a row, so a regression there (favoriting the same mod twice instead of toggling it back off) had no test to catch it. Verified against a real regression: dropping favMods from onToggleFavMod's dependency array makes the second click push a duplicate modid instead of removing it, and this test catches that exact failure (favMods ends up [123, 123] instead of []). Found in review on PR #169.
Neither rules-of-hooks nor exhaustive-deps was ever enforced here, so onToggleFavMod's stale favMods closure and the earlier hasInstallation fix could both have shipped without a lint error to catch them. A trial run against the current tree turned up 21 pre-existing exhaustive-deps violations across 13 files, all real gaps rather than false positives, but each represents a design decision made before this rule existed and fixing 21 dependency arrays sight-unseen risks introducing the exact stale-closure bugs the rule exists to catch elsewhere. Those 13 files keep exhaustive-deps at a warning; everywhere else, including new code, it is an error. ListMods.tsx is deliberately left off that list since it was already being touched by this same PR: its 3 pre-existing violations plus the scroll-listener cleanup's ref warning get inline disable comments with reasons instead, matching the eslint-disable convention that already exists elsewhere in this codebase. Found in review on PR #169.
|
All three blockers are fixed, each in its own commit, each verified against a real regression rather than trusted on read:
Body updated with the disclosure you asked for: queued tasks already hold the close-blocker before they reach the limiter (no separate "queued" state in the UI), only 3 of 5 worker spawn sites are capped ( All gates green on the current head: typecheck, lint:ci (0 errors, 17 warnings, all inside the down-scoped list), format:check, test:coverage (107 files, 1218 passing, 2 skipped, coverage above floor on all four axes), build:unpack. On the follow-ups, since you flagged them as non-blocking rather than required for this PR:
Not fixing any of these five in this PR. Flagging them in the private tracking doc as open follow-up work so they don't just evaporate once this merges. |
Pixnop
left a comment
Pixnop
left a comment
There was a problem hiding this comment.
Approving. All three blockers are fixed and were verified by mutation rather than by reading: the post-acquire re-check kills its test when removed, both limiters' shutdown wiring is load-bearing (dropping either one fails its handler test), deleting the memo on ModListCard fails the new Proxy-counter test with a real number, and removing favMods from the deps fails both the round-trip test and lint:ci, so the belt and the braces both hold.
The worker pool that arrived with the rework got the same treatment and it holds up: twelve of fourteen mutations killed, no module-level state in any of the five worker logic paths (the load-bearing audit, since reuse is real), message correlation by token proven against a deliberately stale straggler, dead workers never returned to the pool, and the installer timeout plus tree-kill untouched. And I closed the one unknown the automated suite cannot reach: built this branch with build:unpack, launched the packaged binary, and drove two consecutive extractions through the pool over its real IPC. Both succeeded with correct trees on disk, so the rewritten worker loading and the postMessage protocol work under the production bundle, not just under vitest.
Three small corrections for a follow-up commit, none blocking. The dequeue-refusal in release() is unreachable by construction (shutdown() empties the queue and acquire() rejects on arrival), so the comment presenting it as the third load-bearing mechanism is wrong; keep the line as cheap hardening if you like, but the comment should say what is true. The terminateAll test acquires and releases the same worker, so it never actually holds an idle and a busy worker at once; acquiring two before releasing one makes it mean what its name says, and idle-worker termination at quit deserves a real pin since nothing else covers it. And the body's one global FIFO wording is now wrong in the other direction: there are two lanes that do not block each other; the head-of-line point stands within the download lane only.
For the record from both review rounds: WORKER_POOL_MAX_IDLE's stated invariant overshoots (extract and compress keep separate idle lists sharing one limiter, so idle can reach 3 where busy caps at 2), the two uncapped call sites now surface the pool's raw shutdown message rather than the localized one, the five worker shims sit at zero coverage, the module-scope icon memory cache still never clears, and the old mock-wrapping memoization test still passes without the memo, worth deleting so the next reader is not reassured by it.
PR #169 recompressed this file but left it at 3840x2160, and the review pointed out that the dimensions are where the memory actually is: Chromium decodes to the intrinsic size, so every window paid for a 4K bitmap no matter how small it was. The decoded ceiling drops from 31.6 MiB (3840*2160*4) to 14.1 MiB (2560*1440*4). Re-exported from the pre-#169 quality-99 original recovered from git history rather than from the shipped quality-76 file, which avoids a second generation of JPEG loss. Lanczos in linear light, then quality 92 with 4:4:4 chroma. Measured at 40.49 dB PSNR against an uncompressed Lanczos downscale of that original, matching the 40.53 dB bar #169 held itself to. File size 1,581,834 -> 1,381,147 bytes. 2560x1440 lands 1:1 on a maximized window on a 1440p panel and still has headroom for a 1280-wide window at devicePixelRatio 2. A maximized window on a 4K panel upscales 1.5x, which sits under the 2px backdrop-blur the root div already applies over this image.
2 participants
Summary
Analysis of the Electron/React engine turned up 10 performance opportunities, ranked by impact vs effort. This PR ships 8 of them. The 2 left out (a full migration of list-item animations from
motionto plain CSS orLazyMotion, and real list virtualization withreact-window/@tanstack/react-virtual) both touch visible rendering and animation behavior directly, and this environment has no working way to verify a change like that visually (a Playwright-driven Electron window would not connect here, tried it, gave up after several failed attempts). List virtualization also depends on the motion migration being done first: removing an item from the DOM whileAnimatePresencestill owns its exit animation means every scroll past the virtualization boundary would fade cards out and back in, a visible regression, not an optimization. Both are better done as their own follow-up PR with a real visual review.ModListCard. The mods grid can hold hundreds of cards, and none were memoized. AddedReact.memoplus a stable callback chain, since memo alone did nothing until the callbacks stopped getting a new identity on every render. Measured with a render-count spy: 2 renders down to 1 for aListModsre-render unrelated to any given card.cachemodimg:protocol handler hit disk and callednet.fetchon every request. Icon file names are content-addressed (the sha256 of their own bytes), so a byte cache needs no invalidation logic at all, just LRU eviction under a 16MB budget. Measured: 1 disk read per icon instead of one per request.content-visibility: autoto installed-mod rows.ModListCardalready used this to skip paint/layout for cards scrolled out of view. The installed-mods list in ManageMods can run just as long (it's a user's own Mods folder), so it gets the same treatment now.ipcRenderer.invokehas no cancellation primitive without adding a whole new IPC surface, which would have been a bigger change than this warranted.validateSevenZipArchive's parse (up to 100,000 entries over 4MB of listing text) used to run in theEXTRACT_ON_PATHIPC handler, on the main process's event loop, before the extraction worker even started. It's now the first thing the extraction worker itself does, which is where it belonged anyway since it was already the first of two validation gates by design.GridItem'suseInViewusedonce: false, motion's own default, so the IntersectionObserver it creates per card never got unobserved and kept firing on every scroll in and out of view. Switched toonce: true; motion's owninView()implementation callsobserver.unobserve()on it once seen, a documented library behavior, not something this change invents. One visible difference: a card scrolled past once no longer re-fades in on a later re-entry, it just stays visible.workerDatais read once at module load and is fixed for a worker's whole lifetime, so reuse meant redesigning all five worker scripts around apostMessage-based task protocol instead. A genuine trade-off, not a pure win: idle workers now cost resident memory the old design avoided, sized conservatively (a handful of idle workers at most, a 30 second idle timeout, retirement after 50 tasks regardless).Fixes from review
Three things pixnop flagged as blocking, fixed here:
before-quitfires used to be able to reach the front of the line and start writing to disk after the app had already begun quitting, since releasing a slot handed it straight to the next queued task with no check for shutdown in between.ConcurrencyLimiternow has a one-wayshutdown(): every queued task rejects the moment it runs, and anything that callsrun()afterward rejects on arrival instead of queueing.WorkerPool.acquire()gets the matching fix, since it had the same gap: it could still hand out a freshly spawned worker afterterminateAll()had already run.ModListCardmocked the component's own module and wrapped the mock inmemo(), which only proved memo works on a spy. Droppingmemo()from the real component left that test green. Replaced with a test that imports the real, unmocked component and wraps themodprop in aProxythat counts property reads: since only the component's own render body ever reads a property off that object, the count only advances on an actual re-render, not onmemo's own reference check.onToggleFavModhad no test exercising a plain favorite-then-unfavorite click sequence on the same mod.configReducer'sADD_FAV_MODpushes ontofavModswith no de-duplication, so a stale closure there would silently double-favorite a mod instead of ever un-favoriting it; there's now a round-trip test that would have caught that.Also turned on
eslint-plugin-react-hooks(rules-of-hooksandexhaustive-deps, both as errors), which the repo had never enabled. It found 21 pre-existing violations across 13 files; those are left as warnings for now rather than fixed sight-unseen in this PR, since guessing at 21 dependency arrays risks introducing exactly the kind of stale-closure bug the rule exists to catch. New code, including everything in this branch, is held to the error level.A few behaviors worth calling out plainly rather than leaving implicit:
DOWNLOAD_ON_PATH,EXTRACT_ON_PATH, andCOMPRESS_ON_PATHare capped.CHANGE_PERMSand the installer-payload extraction underRUN_INSTALLERgo straight to a worker with no limit, since both run once per install with nothing behind them to burst.before-quitfires now rejects immediately with a clear "cancelled because the app is quitting" error, rather than either finishing after the window is already gone or sitting abandoned with no resolution. This path does not run through the main window's own close button, which already checks the same close-blocker and never letsbefore-quitfire while a task is active; it matters for the other ways the app can quit (the tray, an OS session ending, update-and-restart).Type
Checklist
dev, notmain.npm run typecheckpasses.npm run lint:cipasses.npm run format:checkpasses.npm run test:coveragepasses, coverage at or above the floor invitest.config.ts(lines 92.48% vs an 89% floor, statements 90.75% vs 87%, functions 89% vs 85%, branches 87.73% vs 85%).npm run build:unpackpasses.Testing
107 test files, 1218 tests passing, 2 skipped (pre-existing, unrelated to this change). Every item above that could reasonably be measured has a test proving the actual improvement, not just that the code runs: a render-count spy for the memoization fix, a disk-read counter for the icon cache, an integration test firing 4 concurrent downloads to confirm the concurrency cap, a stale-response race test for the search cache, and a full suite for the worker pool (its own bookkeeping against a fake worker factory, the worker-side message protocol against a fake
parentPort, and one test against a real, unmockedworker_threads.Workerproving the one assumption no mock can check: that a worker with a message listener attached really does stay alive between tasks). The two items that aren't measurable in a jsdom/vitest environment (the image compression and the content-visibility change) are backed by objective external numbers instead (file size and PSNR for the image; the same technique this codebase already trusted forModListCard, just applied one component further).The three review fixes above each have their own test, and each was checked against a real regression before being trusted: removing
memo()fromModListCardfails the new memoization test, droppingfavModsfromonToggleFavMod's dependency array fails the new favorite round-trip test (it ends up[123, 123]instead of[]), and a task still queued whenshutdown()runs is asserted to reject with the app-is-quitting error and never start.Also built the Linux AppImage and the unpacked electron-builder package from this branch at each major step and ran them manually, including a check that the worker pool change didn't break app startup with a real packaged build (ASAR, fuses, the works), not just the dev build. That manual pass covered the original 8 items; the review fixes above are checked by
build:unpackand the automated suite only, not by running a packaged build by hand.Related issues
None filed; this came out of an ad hoc performance review, not a tracked bug.