Conversation
Pixnop
left a comment
There was a problem hiding this comment.
The gate is clean at 5976b38: typecheck 0 errors, lint:ci 0 errors and 14 pre-existing hook warnings, format:check clean, test:coverage 236 files / 4463 tests / 0 failed at 91.79 / 88.45 / 93.19 / 93.63 percent stmts/branch/func/lines, all above the 89/87/85/85 floor. Domain mutation testing on src/domain/worlds/worlds.ts killed all five targeted mutants (isSafeWorldName's length boundary, hasWorldSidecars' some/every, collisionFreeWorldName's suffix start, worldVersionWarning's and canTransferWorld's comparison direction). A live headless build confirmed every path reachable through window.api.worldsManager holds byte for byte where checked: list, backup, restore, copy, move, delete, and the unhappy paths too (vanished world, active sidecars, a concurrent race, a corrupt or truncated archive, a read-only destination, path traversal in a world name, bogus installation ids). All of that is real. Two blockers sit underneath it, both in code and UI wiring the coverage never reaches.
src/ipc/handlers/worldsHandlers.ts is the whole 318-line handler behind backup, restore, delete and transfer, and it has 0 percent coverage on every metric. I reran test:coverage scoped to it alongside the seven suites closest to this feature (domain/worlds, ipc/compression, ipc/installationActivity, ipc/workerHost, domain/config/migrations, security-boundaries, ipc-validation) and got the same number directly: 0 of 240 statements, 0 of 186 lines, uncovered 19 to 318, the entire file body. Deleting the same-installation transfer guard at line 250 and rerunning that whole suite still passes clean. Before this merges it needs handler-level tests for the four channels, starting with the guards issue #465 names by name: an unmanaged path, a path with a separator, a world name that is not one of the listed files.
makeWorldBackup (worldsHandlers.ts:144) reads config once at the top of the request, awaits a real compression, then builds nextConfig from that stale snapshot and saves it. Run two world backups for two different Installations close enough together and one backup record is silently lost: the slower save overwrites the faster one's already-persisted worldBackups entry, even though the handler already returned ok:true for both, and the archive file itself stays on disk, orphaned, never listed and never restorable. The SAVE_CONFIG reconciliation that is supposed to protect worldBackups from exactly this (configHandlers.ts:24) is equally untested, dropping it still passes the whole config suite. This needs a proper read-modify-write around the save, and a test that would fail without it.
remove() in ManageInstallationWorlds.tsx:59 gates the delete on window.prompt(), which Electron has never implemented. It returns null with no dialog shown, so the !== world.name check is always true and the function returns before ever calling delete, on every platform, every time. Confirmed live in the packaged build, not only by reading the code. That makes "deletes with confirmation" false as shipped: nothing deletes. Swap it for the PopupDialogPanel confirm the rest of the app already uses for destructive actions, the same pattern ListInstallations.tsx uses for deleting an Installation, and add a renderer-dom test for this component, since nothing currently touches it.
Two smaller things worth fixing now that they surfaced. canTransferWorld in src/domain/worlds/worlds.ts:70 is exported and unit-tested, but transferWorld never calls it: it re-implements the same same-installation check inline at worldsHandlers.ts:250, inside the file with zero coverage. Wire the handler through the domain function it already has a test for, or drop the domain function, having both and running only one is just confusing. And pt-BR.json's new worlds block landed inside backups instead of beside it: line 321 opens worlds right after manageBackups with no closing brace for backups first, and it only closes at line 355, right before backupsDisabled, an existing backups key. features.worlds is entirely absent from pt-BR, the strings are live at features.backups.worlds instead, so every string this page asks for under features.worlds.* falls back to English for pt-BR players. The parity test only requires fr-FR to match en-US so this stayed green. Worth fixing before #497 lands, since that PR reseeds lagging locales from en-US and will draft fresh machine translations here instead of picking up what Zaldaryon already wrote, once it sees features.worlds as entirely missing.
Two items that can ship now and follow up later. World backups have no cap and no delete affordance: makeWorldBackup prepends to worldBackups unconditionally with no equivalent of the backupsLimit pruning installation backups already have, and ManageInstallationWorlds.tsx has no button to remove a single world backup the way ManageInstallationBackups.tsx does for installation backups. Issue #465's own open questions flagged this risk, every world backup taken here is permanent short of deleting the whole Installation. And addWorldBackupRecords (migrations.ts:343) bumps the schema to 6 to do exactly what normalizeInstallation already does unconditionally on every load (configManager.ts:360), default worldBackups to an empty array. That is the opposite of the convention this file documents and configManager.test.ts pins for four earlier fields, none of which needed a bump. Not a functional bug, just extra surface with nothing that would fail if the migration were deleted.
A few more limits worth knowing, none of them blocking. validateWorldBackupArchive's world-name-match check (archiveValidation.ts:172) has no test for a single-entry archive whose entry name mismatches the requested world, impact is limited since restoreWorld renames on extraction anyway. The migration's actual transform (migrations.ts:350) is the same gap as the schema-bump point above, never exercised directly. Copying into a target Installation whose folder was deleted from disk quietly recreates a bare Saves tree and reports success (worldsHandlers.ts:270), since assertConfiguredInstallationPath only checks config, not disk. And the docs line about world changes being disabled while playing is not backed by the UI: the four action buttons stay enabled regardless of Installation activity, though the IPC layer itself does refuse with installation-playing underneath.
# Conflicts: # src/renderer/src/locales/pt-BR.json
Translation statusen-US is the source and carries 905 keys.
The status page is out of date. Refresh it with Drafted values are the machine-drafted ones still waiting for a native review, listed per locale in |
|
Implemented Pixnop's requested changes on this head.
Validation passed locally:
The full local coverage run still encounters existing Node 26 jsdom and TLS incompatibilities outside this change. The repository Node 22 CI remains the merge gate. |
Pixnop
left a comment
There was a problem hiding this comment.
Four of my five requests are met on d6b8be8, and I verified each one.
Coverage of src/ipc/handlers/worldsHandlers.ts goes from 0 to 79.03 statements and 90.20 lines, with guards pinned at tests/ipc/worldsHandlers.test.ts:130 (untrusted sender), :164 (unmanaged installation path), :176 (unlisted world name), :193 (separator in the name) and :237 (refusal while playing). transferWorld routes through canTransferWorld at worldsHandlers.ts:264. window.prompt is gone: ManageInstallationWorlds.tsx:210 is a PopupDialogPanel whose Delete stays disabled until the name matches exactly (:224); on the packaged build Escape closes it, and the confirmed delete removed the file and left the backup restorable. pt-BR carries features.worlds beside backups, pinned at tests/i18n/i18n-parity.test.ts:190. Suite is 4532 passing; log-provenance, text-contrast, security-boundaries and i18n green; the two new log lines carry fixed text only.
Two points block.
-
src/ipc/handlers/configHandlers.ts:24. The SAVE_CONFIG reconciliation I named by line is still untested: replace it with
worldBackups: installation.worldBackupsand the whole suite passes. The renderer does dispatch EDIT_INSTALLATION with its own worldBackups array (ManageInstallationWorlds.tsx:57), so this is live code with no test. -
src/ipc/handlers/worldsHandlers.ts:126. The serialization is not pinned: swap
worldBackupConfigWriteQueue.then(forPromise.resolve().then(and worldsHandlers.test.ts still passes 14/14. The test at :274 only catches the stale snapshot, because the setImmediate at :289 separates the two releases; remove that line and it fails with ['Second.vcdbs'].
Should fix, all deletable with the suite green: validateWorldBackupArchive at worldsHandlers.ts:218 and the single-entry check at :227 are mocked away in the only handler test; the listing test asserts lastModified against itself (tests/ipc/worldsHandlers.test.ts:156), so hardwiring worldsHandlers.ts:93-95 survives; assertManagedPath at :150 and :286 plus assertManagedDeletionPath at :185 are unpinned.
Notes: five window.confirm calls remain (ManageInstallationWorlds.tsx:51), eleven locales are 31 keys short, and docs/get-started/usage/game-client/worlds.md:12 still promises buttons disabled while playing.
|
Addressed Pixnop's review feedback on 8e8a630. Blocking points resolved:
Should-fix items resolved:
Validation passed locally:
|
Summary
Adds per-installation world management for
.vcdbssaves: list world metadata, create and restore individual backups, delete worlds, and copy or move worlds between installations. All filesystem operations are resolved from configured installation and backup records; renderer input never supplies an arbitrary path.The implementation also refuses unsafe world names, active SQLite sidecars, operations while Vintage Story is running, and invalid world archives. Transfers use collision-safe names and report cross-version warnings. Restore uses staging and rollback, and world archive work prevents application shutdown until it finishes.
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.npm run build:unpackpasses.Testing
npm run typecheck: passed.npm run lint:ci: passed with the repository's existing 14 React hook warnings.npm run format:checkandgit diff --check: passed.npm run build:unpack: passed.npm run test:coverage: 233 files and 4,460 tests passed; 3 existing renderer tests failed inactivityCenter,modProfiles, andmodsBrowseSelection. No failure was in the issue Manage an Installation's worlds: list, back up one on its own, move, delete #465 focused tests or changed world paths.Related issues
Fixes #465