Conversation
userData was pinned to appData/VSLauncher so the fork could read an existing VS Launcher install's config on first run. Since v1.7.0-beta.1 RiftLauncher carries its own appId and can sit next to an installed VS Launcher, so sharing that folder means two applications writing one config.json. The launcher now uses appData/RiftLauncher. When only a VS Launcher folder is there, config.json and Icons are copied over once, into a temporary sibling that is renamed into place as the last step, so an interrupted copy can never be mistaken for a finished one. Logs and Cache stay behind: they rebuild themselves. Nothing is renamed or deleted on the VS Launcher side. The decision itself is a pure function in src/domain/userData, and the file work lives in a small main-process adapter, both covered by tests that run against real directories.
windows-conformance.yml covers the install path from source: it imports the extractor out of src/ and runs it under tsx. Everything that only exists once electron-builder has packed the app stays uncovered there, and that is the layer issue #119 reports on: worker modules resolved through the build output rather than src/, app.asar and asarUnpack deciding what a bundled path resolves to, and the electron-builder.yml fuses, which only apply to the packaged binary. tests/e2e/packaged-windows-install.ts launches the packaged executable with a DevTools port open, attaches over CDP and calls the launcher's own preload API from inside its own renderer, so the download and the unpack travel the same contextBridge, ipcMain handlers, path policy and worker spawn a player's click travels. It mirrors AddVersion.tsx one call at a time: getConfig for defaultVersionsFolder, formatPath for the install folder, downloadOnPath, then runInstaller. The download step is load-bearing rather than scaffolding, since RUN_INSTALLER's assertVerifiedArtifact only knows about files DOWNLOAD_ON_PATH recorded in the same main-process session. The DevTools client is written against Node 22's built-in WebSocket so the script needs no new dependency. The report is written after every phase and from the finally block, with the app's stdout and stderr carried in it, because a packaged app that never reaches its window leaves nothing else behind.
Three of them are the same S6544 finding, a promise used as a boolean condition, and all three turn out to be nullable-promise memo lookups rather than forgotten awaits. They now compare against null or undefined explicitly, which says what the check is actually for and keeps the behaviour identical. The other three: the mod change summary no longer prints a bare "0" into the actions cell when an entry carries assetid 0, the ModDB query filters iterate with forEach instead of a map whose array went nowhere, and the stray class-name string sitting at the bottom of PopupDialogPanel is gone.
…progress 100 (#113) Download, extraction, compression and installation all relied on a progress event landing exactly on 100 to move a task to "completed". A run whose last tick came in under 100, or which never sent a terminal tick at all, left the task manager showing finished work as still running. Each flow now dispatches its own completion once the call it awaited resolves, so progress events are cosmetic. UPDATE_TASK becomes idempotent to go with it: an update that changes no field returns the same state array, so the completion arriving second (the listener's or the success path's) is a real no-op instead of a second identical render. Failure paths are untouched.
The shipped CSP allowed inline styles, which SonarCloud flags as a vulnerability. The built renderer does not need it: tailwind arrives as a linked stylesheet, and the one React style prop in the tree is applied through the CSSOM, which the policy does not govern. The vite dev server does need it, because it hands the compiled stylesheet to the page as an inline <style> tag for hot reloading. A small serve-only plugin adds the keyword back for `electron-vite dev`, so index.html keeps the strict policy that ends up in the build. Also adds the lang attribute and a page title, two other Sonar findings on the same file.
canAutoUpdate refused every Linux run without APPIMAGE set, which includes deb, even though electron-builder.yml publishes deb and electron-updater ships a DebUpdater for it. The DebUpdater loads once electron-updater reads the package-type marker electron-builder writes next to the packaged app, so canAutoUpdate now takes that marker as input and allows deb the same way it already allows AppImage. Flatpak still refuses: it has neither marker and updates through its own repo.
feat(main): move user data to appData/RiftLauncher with a one-time copy
ci(e2e): prove a version install through the packaged Windows app
…lation pass
Paulo Nascimento's pt-BR.json covered 290 of en-US's 351 keys. The other
locales are all at the same 290/61 split, so no existing translation had
these strings to draw from either; translated directly from en-US, cross-
checking terminology against the already-translated 290 keys in this file
and the same sections in ru-RU.json, pt-PT.json and es-ES.json to keep verb
register, loanwords ("Mod", "Modpack", "Backup") and punctuation consistent
with what was already established. pt-BR is now the only locale at full
parity with en-US (351/351, tests/i18n/i18n-parity.test.ts's coverage
snapshot confirms 0 missing, 0 orphans).
Verified: JSON parses, prettier --check passes, the parity suite's 7 tests
pass, and the strings actually render: built the packaged app, forced
pt-BR via the same localStorage path the Config page's language picker
uses, and screenshotted the Home and Info & Help pages plus the trailer
thumbnail's alt/title text.
Paulo Nascimento's original translation still makes up the majority of the file (290 of 351 keys); this adds Zaldaryon for the 61-key catch-up, following the multi-contributor format pl-PL already uses.
…rsion on edit Opening the edit page for an Installation whose VS Version was uninstalled picked gameVersions[0], whatever sat first in config order, and wrote it to disk on the next save with no notice. A player renaming an installation could end up running a different game version than the one their worlds were created on. EditInstallation now leaves the version unselected instead of substituting one, GameVersionPicker shows an inline warning naming the missing version (same banner PR #87 already established for the version-in-use case), and saving without a version refuses with the same message MainMenu already uses when Play hits an orphaned installation. Also dropped the form's initial-state fallback (a semver-sorted default that never survives past the mount effect), which drops the now-unused semver import. Closes #118. Deliberately out of scope: a marker on the installations list for orphaned entries (ListInstallations doesn't subscribe to game versions today, and the orphan is already surfaced at both moments that matter, launch and edit), and any repair of installations already silently reassigned by this bug before the fix, since there is no way to tell a repaired guess from a deliberate version change.
… list Nothing on the installations list said an Installation's VS Version had been uninstalled. The player only found out at Play (MainMenu's versionNotInstalled refusal) or Edit (#126's fix), never by looking at the list itself. ListInstallations now checks each Installation's version against the currently installed gameVersions and, when it doesn't match, shows the version in orange with a warning icon and the same versionNotInstalled tooltip text the other two surfaces already use, rather than adding a new key for a fourth wording of the same fact. Closes #127.
…logger Every log line this app writes goes through redactSensitiveText, which strips credentials, tokens and absolute paths before they reach disk. The auto-updater was the exception: it was handed the raw electron-log instance, so its cache paths, feed URLs and error stacks landed in the log file verbatim, complete with the user's home directory that absolutePathPattern exists to remove. createUpdaterLogger routes each level through logMessage instead, keeping the documented "just set logger" wiring while restoring the redaction pass. It takes its log function as a parameter so the test can assert the level mapping without mocking a module. The assignment also moves below the resolvePathFn block. The line it replaces logged before that block ran, so it landed in electron-log's default path rather than the app's own Logs directory. The hand-written error listener goes with it. AppUpdater's constructor already attaches one that logs error.stack || error.message through the configured logger, which is more than the message this one carried.
The launcher has no prose input and no context menu anywhere in the app, so spelling suggestions were never reachable. What it did cost was real: a fresh profile downloads a hunspell dictionary from a third-party CDN into userData/Dictionaries at startup and keeps the spellcheck service alive for it. webPreferences.spellcheck: false alone did not stop this (a known Electron bug, electron/electron#22995 and #24931): measured 3,923,495 bytes (pt-BR-3-0.bdic) downloaded on a clean profile with only that flag set. Adding session.defaultSession.setSpellCheckerEnabled(false) and clearing setSpellCheckerLanguages([]) actually suppresses it: verified over three separate clean-profile launches (one 15s run) that userData/Dictionaries stays empty.
ManageMods and the ModDB browse grid can both mount hundreds of installed/downloadable Mods in a single session (ManageMods.tsx maps its full scanned list, ListMods.tsx grows visibleMods by 10 per scroll tick with no upper bound). Every card's logo image was loading eagerly, so scrolling through a large list downloaded and decoded images the user never looked at. Native lazy loading defers each image's request until it nears the viewport, which the browser already does for free.
Every card in the ModDB browse grid still paid full layout and paint even while scrolled far out of view, because the grid renders its whole visibleCount slice at once and none of it unmounts. Marking each card's two inner panels (the logo area and the stats/summary area) with content-visibility: auto lets the browser skip layout, style recalculation and paint for panels outside the viewport, the same benefit list virtualization gives, without unmounting anything that AnimatePresence and useInView (Grid.tsx, Table.tsx) still need mounted to animate. The outer GridItem frame keeps its own layout, since content-visibility containment can clip a backdrop-blur/shadow that crosses its box. Measured on the packaged build at the first 45 cards mounted: LayoutObjects 2292 to 638, initial image requests 29 to 6 (auto containment also defers imagery below the fold, on top of loading= "lazy"). After scrolling through all 405 cards, steady-state layout work during further scrolling goes up slightly instead of down, since each newly revealed batch now pays its layout cost on scroll rather than all upfront. That is an expected trade-off of deferred layout, not a regression: the common case is opening the Mods page and browsing the first screen or two, not scrolling through hundreds of entries in one sitting.
…ation and stop freezing the whole form configManager normalizes a missing or invalid installation version to the empty string and keeps the installation, so version: "" is a reachable config state. The warning banner was guarded with missingVersion &&, which is falsy for an empty string, so an installation in that state showed an empty picker with no explanation at all, and the save guard refused with a blank "VS Version not installed!" message. The guard is now missingVersion !== undefined, and the empty case gets its own copy instead of interpolating nothing into a sentence about a named version. MainMenu's Play guard had the same blank interpolation for that state, so it now branches to a short version of the same message. versionNotInstalled keeps its copy and meaning for the named case, so no other locale is affected. Saving no longer requires a version. The version key is left out of the EDIT_INSTALLATION payload when the picker has no selection, so the reducer's Partial merge keeps whatever the installation already points at, orphaned or unset, and renaming or changing backup settings works the way it did before this fix. The point of this fix is that an edit must never reassign the version, not that an orphaned installation must become read only, and freezing the whole form took away something players could do yesterday. The state still gets stated twice: the banner stays on the page, and a warning toast on save says the installation still has no installed VS Version. Three new tests cover the empty-version state (the banner that never rendered, saving the rest of the form without ever writing a version) and the play refusal message. The existing save-refusal test became a partial-save test, still failing against the original fix's own source for the reason it always did: that save used to overwrite the version.
fix(updater): allow auto-update on deb installs
…-always fix(updater): route electron-updater's logging through the redacting logger
i18n(pt-BR): catch up the 61 keys added to en-US since the last translation pass
feat(installations): mark an orphaned VS Version on the installations list
…ygiene perf(renderer): stop wasted spellcheck download, lazy-load mod images, skip off-screen grid layout
PR #129 stopped the Chromium spellcheck dictionary download with three lines: webPreferences.spellcheck: false, setSpellCheckerEnabled(false) and setSpellCheckerLanguages([]). The comments credited the enabled toggle as the session-level call that stops the fetch and described the empty language list as covering the case where the fetch follows the OS locale. That is backwards. Pixnop A/B tested it while reviewing #129: with the flag and setSpellCheckerEnabled(false) set and the language list left alone, a fresh profile still downloaded the dictionary, reproduced twice. Clearing the language list is what stops it, because the fetch follows the session's language list. Read the old way round, setSpellCheckerLanguages([]) looks like a redundant line worth tidying up, and deleting it restores a multi-megabyte download from a third-party CDN on every fresh profile with nothing to catch it. So the comments now say which call carries the fix, and a new block in tests/security-boundaries.test.ts reads src/main/index.ts and fails if either session call goes missing. It reads the source because index.ts runs app.whenReady() on load and cannot be imported into a test, the same reason accountLoginFlow.test.ts greps accountHandlers.ts. No behavior change: both session calls run exactly as before, in the same order.
…edit docs(main): credit the call that stops the spellcheck download, and guard it
…an-version fix(installations): stop reassigning an Installation to another VS Version on edit
The first slice of the #107 triage, taking the findings where the smell is a failure nobody would see and leaving the stylistic ones alone. Read off the live analysis rather than the issue text: 206 findings are open, all code smells now that #112 cleared the five bugs, and the shape of the list is 66 read-only props, 48 `node:path` imports and 18 `toHaveLength` preferences before anything that can cost a real failure. Four tests had no assertion in them, the only BLOCKER-severity findings in the project. Three in archiveValidation call `validateArchive` bare and fail only because a rejection propagates out of the test body, so the day that function returns a verdict instead of throwing, all three would pass while checking nothing. They say `assert.doesNotReject` now, matching how the refusal cases in the same file already read, and the fourth site in that file gets the same treatment even though Sonar did not flag it, since its trailing `assert.rejects` is what hid it. The EditInstallation case is the weakest of the four and does fail on a miss through `findByText`, so it gets the explicit assertion the neighbouring tests write rather than a behaviour change. AddVersion's presence check moves from `queryByTitle` to `getByTitle`, so a missing Reload button fails with the DOM instead of "expected null to be truthy"; the absence check next to it stays on `queryBy`, which is what `queryBy` is for. Three catch blocks showed the player a notification and dropped the error on the floor, which is the difference between a bug report saying "it said error adding installation" and a log line naming the call that threw. They now log the error and debug pair the same folder already uses in ManageInstallationBackups. Refs #107
…#154) * fix(versions): handle invalid semver strings in the version list sort semver.rcompare throws when either argument is not a valid semver string. A registered version with nonstandard probe output (pre-release builds, modded launchers, or parse failures) crashed the entire versions page. Check semver.valid() before comparing. Valid versions sort among themselves with rcompare as before. Invalid ones sort last, ordered alphabetically so the list stays deterministic. Fixes #148 * test(versions): pin the non-semver sort guard with a DOM test Seed a version entry with a non-parseable string ("Vintage Story 1.21.0") and assert both rows render. This test fails against dev, where semver.rcompare throws on the invalid string and crashes the page.
* fix(backups): prevent duplicate backup deletes * fix(backups): guard _deleting clear on backup-in-use, assert deletePath call count The failure path at ManageInstallationBackups:107 cleared _deleting unconditionally. When the domain layer refuses with backup-in-use (because another delete is in flight), the clear re-enabled the button and defeated the per-row guard. Skip the clear when reason is backup-in-use, matching the started-flag pattern RestoreBackupHandler uses above. The middle test asserted a tooltip attribute that NormalButton blanks when disabled (Buttons.tsx:43). Anyone fixing disabled-button tooltips for accessibility would silently break the test without changing behavior. Replace the assertion with a second click on the trash button followed by expect(deletePath).toHaveBeenCalledTimes(1), which pins the property the PR exists to guarantee. Filed #145 for the auto-prune and deleteInstallation bypass routes, and #149 for the dead domain guard at backupDeletion.ts:28. * test(backups): rewrite the double-delete guard test to fail pre-fix The previous test was vacuous: it found the trash button conditionally and skipped the click when no match existed, so it passed on dev where the button stays enabled. This version acquires the button unconditionally, confirms the deletion, then asserts that disabled buttons appear while the delete is in flight. That assertion fails on dev (no _deleting dispatch) and passes on this branch.
* fix(mods): lower per-icon size limit from 8 MiB to 512 KiB MAX_MOD_IMAGE_BYTES was 8 MiB. The total cache budget is 64 MiB, so eight icons at the maximum legal size filled the entire cache by themselves. No real mod icon is anywhere near 8 MiB: typical modicon.png files weigh tens of kilobytes. Drop the threshold to 512 KiB. Any PNG above that is not a reasonable icon. This keeps the cache budget meaningful: 128 maximum-size icons fit now instead of 8. Fixes #146 * fix(mods): skip the icon instead of erroring the mod when it exceeds the size cap An oversized modicon.png now costs the mod its picture, not its place in the list. The limit drops from 8 MiB to 512 KiB (a reasonable ceiling for a mod icon), and both the declared-size guard and the runtime-size callback advance past the icon entry instead of settling the archive as an error. Tests updated: the oversized-declared-icon fixture now expects a successful read with icon undefined, and the domain-level test asserts the mod lands in mods without an icon rather than in errors.
…153) * fix(versions): normalize folder paths before the foldersInUse check The folder-in-use guard used Array.includes(), which does raw string equality. A trailing slash, different casing on Windows, or mixed separators let the user register two versions pointing at the same physical directory. Introduce normalizeFolderForComparison() in src/domain/paths.ts: strips trailing separators and lowercases on Windows (detected by drive letter or backslash presence). Replace the raw .includes() in both installGameVersion and createInstallation with folderIsInUse(), which normalizes both sides before comparing. 12 new unit tests cover the normalizer and the comparison helper. Fixes #147 * fix(versions): unify separators and accept platform param for path comparison Replace the looksLikeWindows heuristic with an explicit platform parameter (defaults to auto-detect from drive letter). Unify backslashes to forward slashes before comparison, so C:/Games/VS and C:\Games\VS match correctly. On posix, a backslash in a filename is normalized the same way (unified to /) but case is preserved, preventing the false-positive where two genuinely distinct Linux paths got lowercased into a collision. Tests cover: mixed separators on Windows, explicit posix with backslash filename chars, and the platform param threading through folderIsInUse.
* fix(mods): prune icon cache after each scan, not only at startup The icon cache folder only ran eviction on application startup. During a session, every mod scan wrote new icons without checking total size, so users browsing many installations could grow the cache past its 64 MiB budget indefinitely until the next restart. Call pruneModIconCache() after every GET_INSTALLED_MODS scan completes. The call is fire-and-forget: it reads the folder, plans eviction against the existing budget, and deletes the oldest icons over budget, same logic startup already ran. No blocking the IPC response. Fixes #144 * fix(mods): add re-entrancy guard and mtime check to pruneModIconCache Concurrent calls coalesce into one active sweep plus one trailing re-run. Before removing each file, re-stat it and skip if mtime moved since the snapshot, which closes the race where a scan touches an icon the sweep already planned to delete. Previously the race was theoretical (startup only), but with the sweep now firing after every scan it becomes the normal interleaving. Tests cover: mtime-moved skip, overlapping call coalescing. * test(mods): assert coalescing readdir count and GET_INSTALLED_MODS prune call The coalescing test now spies on fse.readdir and asserts at most 2 calls to the icon folder, proving overlapping pruneModIconCache calls do not each run their own sweep. A new GET_INSTALLED_MODS describe block verifies the handler calls pruneModIconCache after a scan that finds mods, and skips it when the path does not exist. pathPolicy is mocked to isolate the handler shell from config-dependent path grants.
…#138) * feat(versions): register a folder without letting uninstall delete it Pointing the launcher at a folder that already holds Vintage Story has worked for a while: ListVersions links to the look-for-a-version page, LOOK_FOR_A_GAME_VERSION probes the executable with `-v` through detectInstalledGameVersion, and addVersion registers what it found. Nothing recorded where the folder came from, so uninstallGameVersion ran `fileSystem.remove(version.path)` on it like any other version. A player who registered their own `C:\Games\VintageStory` and later pressed the trash button lost that install, under a confirmation dialog that promised the uninstall was not reversible and never said whose folder was about to go. `GameVersionType` gains a persisted `linked`, set only by the look-for-a-version flow. It carries no underscore because it has to survive a save: normalizeGameVersion rebuilds every entry from `version` and `path` alone, so it now carries `linked` through explicitly, and a test covers that, since a flag that holds for one session and disappears on the next launch would be worse than no flag at all. uninstallGameVersion keeps its guard order (playing, busy, in use) and now returns `{ ok: true, folderRemoved }`. A linked version never reaches the removal, so the list drops it and the folder stays. ListVersions says "remove from list" instead of "uninstall" on the button, in the confirmation, and in the in-use warning's second line, which otherwise promised something irreversible about a folder nothing was deleting. The `assets/version-X.Y.Z.txt` marker fallback and the manual version field the issue also mentions stay open. Both matter for a build whose executable this machine cannot run, neither is a data-loss path, and both are a separate change. Fixes #120 * feat(config): add schema 3 migration stamping linked on external versions Existing game versions registered before the linked flag was introduced had no flag on disk. On uninstall the launcher checked that flag to decide whether to delete the folder or just remove it from the list. Without it, a user-provided folder would have been deleted. The schema 2 to 3 migration reads each game version's path and compares it against defaultVersionsFolder. Versions under the managed folder stay unlinked (they were downloaded by the launcher); everything else gets linked: true stamped on it. Also adds a renderer-dom test asserting that useLookForAVersion dispatches ADD_GAME_VERSION with linked: true when registering a folder. * fix(tests): update schema expectations after schema 3 migration Two test fixtures used schemaVersion: 2 as their default config shape. With the schema 3 migration (linked game versions), the float-era document now migrates up to 3, and the win32 installer tests need a valid current schema to avoid normalization re-stamping during config load. Update both assertions to match the new CURRENT_CONFIG_SCHEMA of 3. * fix(config): enforce path boundary in schema 3 migration
Replace icon.png, icon.ico, and icon.icns with the new RiftLauncher logo. Remove the old VS_Launcher_*.png files that nothing references. Add the full-resolution source (riftlauncher-full.png), the PSD source file, and the two ModDB banner variants (extended and squared) under resources/. Copy the compressed icon into src/renderer/src/assets/icon.png so the HTML favicon and any future renderer-side usage picks it up.
On posix, backslash is a legal filename character. The unconditional replacement collapsed dir\x and dir/x into the same normalized string, producing false in-use refusals when both existed. Move the backslash-to-slash replacement inside the win32 branch so posix paths preserve backslash as a literal character. Fixes #155
…-installation (#158) PR #136 wired the per-backup _deleting flag into the manual-delete button's own guard, but two other paths that remove backup archives never learned about it: the auto-prune loop in makeInstallationBackup (backupsLimit enforcement, runs before every new backup) and the backup cleanup inside deleteInstallation. Starting a manual delete on an archive, then triggering either of those before it finishes, raced the same file: the prune loop could report prune-failed for a file another operation had already removed correctly, and deleteInstallation could land a bogus entry in failedBackupPaths for the same reason. Both InstallationSnapshot.backups and InstallationDeleteSnapshot.backups now carry an optional isDeleting flag, filled in by the adapters from the config-owned _deleting field. The prune loop skips a candidate that is isDeleting without touching its file, still counting it toward the target since it is on its way out through the other operation either way. deleteInstallation does the same: skips it, reports it as neither removed nor failed, since its actual fate belongs to whichever operation is handling it. Fixes #142
CONTRIBUTING.md's own guidance is one line ("include a summary of
the changes made and why they are necessary"), and there is nothing
in .github/ to back it up. Stratum has a template; this repository
does not, so every PR body has been reconstructed from the maintainer's
own prose habits rather than prompted by anything on screen.
Adapted rather than copied. Stratum's checklist items are specific
to its decompile-and-patch workflow (extract-patches.ps1, the //
Stratum marker convention, no vanilla source committed) and do not
apply here, so this template's checklist is the five local gates
this repository actually has instead: typecheck, lint:ci, format:check,
test:coverage, build:unpack, the same five branch protection already
requires before merge. Added one line neither template has: a
reminder that normal pull requests target dev, not main, since that
rule exists only in AGENTS.md-equivalent private notes and CONTRIBUTING.md
today, not anywhere a contributor opening a PR would see it.
The Testing section asks for real numbers over a bare "tests pass"
checkbox, closer to how Nimbus's unwritten convention already reads
(test counts, coverage percentages, before-and-after numbers for a
performance change) than to Stratum's checkbox-only shape.
…160) PR #114 moved userData to appData/RiftLauncher and said explicitly what it left out: the three VSL* default folders in configManager.ts hold game installs and backups that can run tens of gigabytes, are user configurable, and are not the same problem as two processes writing one config file. That line is the last open piece of #16. defaultInstallationsFolder, defaultVersionsFolder and backupsFolder now default to RiftLauncherInstallations, RiftLauncherGameVersions and RiftLauncherBackups instead of VSLInstallations, VSLGameVersions and VSLBackups. This only changes what a document with no value in those fields gets: normalizeConfig falls back to defaultConfig only when the stored string is missing or invalid, so an existing config, whatever it already has stored, is untouched. No data on disk moves or gets renamed. A new test locks that guarantee directly: a document that still carries the pre-rebrand VSL paths keeps them exactly, unchanged, through normalizeConfig. Fixing the two configHandlers.test.ts assertions that hardcoded the old default surfaced a real coupling neither of us had in mind going in: SAVE_CONFIG's authorization check grants paths against the live current config, so a fixture still declaring VSL-prefixed roots stopped being an authorized set of paths against a RiftLauncher-branded default and started failing with unauthorized-path instead of the write outcome the tests meant to exercise. Fixed by moving the fixture onto the new names, which is what made those roots self-authorizing again. Fixes #16
…docs (#162) electron-builder's linux.icon pointed at resources/icon.icns, an Apple-only bundle format. It happened to still produce a usable single-size PNG through the icns-parser's undocumented fallback, but skipped the standard hicolor icon set entirely. Switched to a build/icons directory of 9 properly sized PNGs (16 through 512), generated from the same compressed source asset the rebrand already shipped, following electron-builder's documented icon set convention. Also pointed the in-app Guides link at our own docs (already moved into this repo by #59) instead of the old third-party vsldocs site, and dropped that now-unused hostname from the browser URL allowlist.
#161) The auto-prune loop in makeInstallationBackup and deleteInstallation's own backup cleanup both duplicated the isDeleting check that deleteInstallationBackup already centralizes for the manual-delete button, so #149's own guard stayed a single-caller function even after both paths were made safe. Routes both through deleteInstallationBackup instead, which also picks up the isRestoring check neither path considered before: a restore in flight on the oldest backup could previously be pruned out from under it. Fixes #149
The GitHub wiki now has a full player-facing set of guides (install, usage, translation), migrated from the GitBook-flavored docs/get-started/ tree and rewritten in plain Markdown for GitHub's wiki renderer. Point the in-app Guides button there instead of the docs/README.md blob link PR #162 set it to before the wiki existed.
…ils (#164) detectInstalledGameVersion probes a folder's executable with -v; when none of the expected executable names are present, the probe fails, or it runs but prints nothing usable, LookForAVersion cleared the folder and version fields and dead-ended with an error notification. That made it impossible to register a folder whose build genuinely has no readable version, which is exactly the case issue #120 was filed for: unreleased or nonstandard builds tsu and Jessica wanted to point the launcher at. Keeps the picked folder set when detection finds nothing instead of clearing it, makes the version field editable instead of read-only, and softens the notification from an error to an info message inviting manual entry. Registration itself is unchanged: addVersion's existing checks (missing folder or version, already-installed version) apply the same whether the version came from the probe or from typing. Fixes #120
Remove the Info page banner and its locale keys, dedicated renderer test, browser allowlist entry, and stray login comment. Keep the other Info links and login attribution unchanged. Fixes #173.
* perf(assets): compress the background image with CaesiumCLT 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. * perf(mods): memoize ModListCard and stabilize its callback chain 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. * perf(mods): cache mod icon bytes in memory across cachemodimg: requests 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. * perf(paths): cap concurrent downloads/extractions/compressions 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. * perf(mods): skip offscreen layout/paint on installed-mod rows too 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. * perf(mods): cache repeat ModDB searches, ignore stale out-of-order ones 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. * perf(paths): move archive-listing validation into the extraction worker 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. * perf(ui): stop re-animating grid cards on every scroll pass 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. * fix(tests): satisfy strict indexed-access typing in gridItemInView.test.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. * perf(paths): reuse worker threads across tasks instead of one per task 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. * fix: close the before-quit shutdown race in ConcurrencyLimiter and WorkerPool 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. * test: pin ModListCard's real memo() instead of a mocked stand-in 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. * test: cover the un-favorite round trip on the mods list 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. * lint: turn on eslint-plugin-react-hooks 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. * fix: refine worker pool follow-up
* test: cover cachemodimg protocol handling * test: close cachemodimg review gaps
* perf: release mod icon cache after leaving page * test: pin icon cache lifecycle wiring
Zaldaryon
approved these changes
Aug 21, 2026
Zaldaryon
left a comment
Zaldaryon
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed as a release promotion (dev at 66a557d -> main), not a normal feature diff — every underlying commit already went through its own review when it merged into dev.
Checked what's specific to promoting a release:
package.jsonversion:1.7.0-beta.1onmain->1.7.0-beta.2ondev, correctly bumped by #179.- The 4 commits
maincarries thatdevdoesn't (thewindows-conformanceworkflow registration,52f7f8c/0c98d27, plus the two merge commits) are not reverted or altered in a way that changes behavior:ci.ymlis byte-identical between the branches, andwindows-conformance.ymlonly gained a clarifying comment pointing at the newerpackaged-windows-conformance.yml. electron-builder.ymlandrelease.ymldiffs addrpm/pacmanLinux targets and move the app icon fromresources/icon.icnstobuild/icons; the newbuild/icons/directory is fully populated (9 PNG sizes) and nothing still references the old.icnspath.- No leftover AI traces, private paths, or workspace-specific strings in the full
main...devdiff. - All required checks pass on the current head (
typecheck,lint,test,build (ubuntu-latest),build (windows-latest)), plus the informationalsonarcloudandpackaged-windows-install-conformanceruns. mergeStateStatusisCLEAN; no conflicts.
No blocking findings. Release publication (tagging v1.7.0-beta.2) remains a separate step after this promotion merges.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Everything merged since the beta.1 promotion, aligned onto main for the second prerelease. The headline items: the Windows install fix for the false hard-link refusals that hit the first field tester, folder registration with the linked flag and its schema 3 migration so uninstall can never delete a user's own folder, the user-data migration to the RiftLauncher folder (every migrating player signs in again once), auto-update for deb, rpm and pacman, the complete rebrand through to the freedesktop icons and the wiki, the backups deletion guard family, the icon cache rework on both the disk and memory layers, and the perf pass with its worker pool.
After this merges: tag v1.7.0-beta.2, let the release workflow build the artifact set (rpm and pacman for the first time), review the draft, publish as a prerelease, then run the upgrade proof from an installed beta.1.