Name the mods a modpack import is about to touch, and say what it will do to each - #384
Conversation
Zaldaryon
left a comment
There was a problem hiding this comment.
The table this adds is the right feature and most of it holds up. Two things block it, both about a case the change now reaches that it did not before.
A mod name over 256 characters now fails Export Modpack entirely. parseModpackManifest in src/ipc/handlers/modsHandlers.ts validates both directions: IMPORT_MODPACK at line 231 and EXPORT_MODPACK at line 177. toModpackManifest now copies mod.name verbatim into every entry, and the mod scanner reads a modinfo.json name through readText with its default MAX_TEXT_LENGTH of 4096, while the new assertString(entry.name, "mod name", 256) refuses anything over 256. So one installed mod whose declared name is longer than 256 characters makes the whole export fail with { success: false } and a generic notification, no partial result, on an action that could not fail this way before this PR. Raise the cap to the 4096 the scanner already allows, or clamp or omit an over-length name in toModpackManifest rather than failing the export, and add a test with an over-length name.
The "not on the mod database" note states a cause it cannot know, and now shows it eagerly. useQueryMod catches every failure, logs it, and returns undefined, so a transport failure is indistinguishable from a real 404. With the mod database unreachable, every unresolved entry becomes not-on-moddb, and the new note under the table states as fact: "N mod(s) are not on the mod database: no listing there declares the mod id in any of its releases. Those are most likely forks or private builds, and have to be installed by hand." On a transient outage that is wrong, and wrong in a way that sends the player toward a manual install they do not need. Unlike the pre-PR behaviour it now appears on manifest open, before the player has clicked anything. Have the effect tell a lookup that failed from one that answered 404, by reporting the failure out of queryMod or counting failures in the effect, and show a "couldn't reach the mod database" state instead. At minimum, soften the note so it does not assert a cause.
The rest checks out, so a re-review does not need to redo it. The manifest reader is backward compatible with a nameless pack and refuses a wrong-typed name. The eager effect cannot loop: queryMod is useCallback([])-stable and installedMods is component state, so the installed memo identity is stable, and the cancelled flag covers close, reopen and unmount. The QUERY_URL limiter is declared and shut down on before-quit exactly the way the pathsHandlers.ts download and archive lanes are, and queueing cannot burn a request's own timeout because requestBoundedText starts its wall clock inside the task the limiter runs. modpackRowStatus agrees with the planner's own downgrade comparison and handles every skip and install shape, including the #287 disabled copy and a hand-edited version string both landing on replace. modpackRowLabel's resolved-name-equals-modid guard matches the planner's not-on-moddb arm, and the label is a plain JSX text child with no injection surface. I measured the two new hues against the popup table backdrop: text-orange-300 at 9.47:1 and text-blue-400 at 6.28:1, both well over the 4.5 floor.
Two non-blocking follow-ups. The single global FIFO means Manage Mods' still-unbounded installed-mod fan-out can make a later catalog or filter fetch wait behind a full page scan; bounding that fan-out at its own call site would keep one page's scan from starving another page's first request, and a follow-up issue is enough. And text-orange-300 and text-blue-400 should be pinned in tests/text-contrast.test.ts the way #366 pinned the release-table verdicts, since text-red-700 shipped at 2.18:1 precisely because nothing measured it.
Full local gate on 0b661b6 is green: typecheck, lint:ci (0 errors, 15 pre-existing hook warnings), format:check, test:coverage at 93.48% statements and 90.29% branches above the 87/85 floors, build:unpack. The findings above are behaviour, not gate failures.
A modid and a listing name diverge often: tradie is Traders Expansion, sandwich is Sammiches, hqzlights is Braziers. The export wrote the modid and the version and nothing else, so an entry the ModDB cannot resolve had no name the player could recognise, which is what sent NekoJess looking for a mod she already had. Each manifest entry now carries the name off the installed copy's modinfo.json. The reader takes a manifest with names and one without, because every pack exported until now has none, and the field is a label only: the modid stays the identifier. Building the manifest moves out of the export hook into the mods adapter so the shape is pinned by a test rather than by a hook nobody mounts.
The table printed the modid and nothing else, so th3dungeon, terraprety and thetruearchimedesscrew were three rows a player could not map to Ancient Dungeons, Terra Prety and True Archimedes Screw. The name is already in the plan and, since the previous commit, in the manifest too. modpackRowLabel takes the ModDB name, then the name the pack was exported with, then the modid, and a plan item that named itself after its own modid counts as unnamed. The modid stays on a second line whenever the label is not the modid itself, and the row key is still the modid. The ModDB lookups move from the Import click to the moment the manifest opens. They cost the same either way, the import needs them regardless, and having them in hand is what lets the table name a row before the player commits to anything. Import waits on them and says so.
planModpackImport already decides all of it: whether the mod is new, which release was picked, what is installed today, whether that is a downgrade. The table only revealed any of it after the fact, one row at a time, as the downloads ran. modpackRowStatus reads a plan item back as the sentence its row shows: new install, update from one version to another, downgrade, already installed, not on the mod database, no release. The row keeps whatever the import reports the moment an entry starts moving, so what happened always outranks what was going to happen.
Three ids in the reported pack resolve to nothing: alloycalculator- stuzzichino, animationslib, waterwheelriverflowfix. The mods exist, the ids do not. The hub's Alloy Calculator listing declares alloycalculator, so what she runs is a fork or a build that was never uploaded there. "Not found on ModDB" read as if the mod did not exist, which is what sent her looking for it. The row now says the mod is not on the mod database, and a note under the table says what that means: no listing declares the mod id in any of its releases, so those copies are most likely forks or private builds and have to be installed by hand.
satisfyingCopy accepts an installed copy only when it is enabled and at the exact version the manifest names, so a mod whose version string the player edited by hand, or one they had turned off, is re-downloaded and the copy on disk is overwritten. "two of them have slightly changed names given I edited them personally" in the report is exactly that case. Nothing changes about what gets installed. The row now says the installed copy will be replaced, and names both versions, so the overwrite is visible before Import rather than after. An update and a downgrade already said so through their own wording; this is the third case, where the two versions match and the copy is replaced anyway.
The modpack import resolves one detail lookup per entry the folder does not already satisfy, and since this branch it does that when the manifest loads instead of when Import is clicked. A 200-mod pack therefore opened 200 sockets at the mod database off a file chooser, and nothing downstream bounded it. QUERY_URL now runs its request inside a six-slot ConcurrencyLimiter, the same way pathsHandlers.ts lanes downloads and archive work, with its shutdown wired to before-quit alongside them. Six is the per-host connection cap browsers use, so a burst from the launcher is no heavier on a community-run site than an ordinary page visit, and 200 lookups in lanes of six still settle in seconds. The bound sits in the IPC layer rather than in the popup because every caller of the channel shares the one host: Manage Mods fans out over installed mods the same way.
0b661b6 to
f4a0d38
Compare
Rebasing onto dev brought in #385's filter-scope test alongside this branch's own name-carrying manifest change, and the two never met before: the test's expected export payload predates the mod name riding along in every entry, so it failed the moment both landed on the same tree. Bring its expectation in line with what the manifest has carried since #379.
parseModpackManifest refuses a mod name over 256 characters on both EXPORT_MODPACK and IMPORT_MODPACK, but toModpackManifest was copying mod.name verbatim, and the scanner reads a modinfo.json name up to 4096 characters. One installed mod with a long declared name failed the whole export with a generic notification, an action that could not fail this way before. A label must never be the reason an export fails. toModpackManifest now clamps the name through a new domain helper, clampModpackModName, which cuts to the same 256-character cap the reader enforces (both now read off one constant, MAX_MODPACK_MOD_NAME_LENGTH, so they cannot drift apart again) and never splits a surrogate pair on the cut. Covered in the domain (the clamp itself, including the surrogate-pair boundary), in the adapter (an over-length name comes out clamped), and at the IPC handler (a name at exactly the cap exports and re-imports cleanly).
useQueryMod caught every failure alike and returned undefined, so a transport failure looked exactly like a 404. With the mod database unreachable, every unresolved entry in the import table became not-on-moddb, and the note under the table told the player those were forks or private builds to install by hand, before they had clicked anything. useQueryMod now returns a discriminated outcome: found, not-found (a genuine 404), or failed (the call threw, or the envelope did not check out as a real 404). planModpackImport takes an optional set of modids whose lookup failed and gives those entries a new skip reason, lookup-failed, kept apart from not-on-moddb. The import popup renders it as "Couldn't reach the mod database" with its own note and a retry action, and replaces the whole table with an unreachable state, retry included, when every lookup in the batch failed rather than showing a table that is all wrong guesses. The two other callers of useQueryMod (the installed-mods update scan and the single-release catalog) still fold not-found and failed together, which is all they have ever needed. Tests cover all lookups failing, a mix of a real 404 and a transport failure, and the retry action re-running the lookups and resolving the table.
text-orange-300 and text-blue-400 joined statusColor's switch as part of the readable-table pass, but nothing measured them the way #366 measures the release verdict hues below, which is exactly how text-red-700 shipped at 2.18:1 unnoticed. Zaldaryon measured 9.47:1 and 6.28:1 on the popup table backdrop; this pins both by computing the ratio the same way the rest of the file does, through paletteForeground and tailwindColor, rather than quoting his numbers.
Follow-up to the previous commit: the four-parameter statusOf helper wrapped across lines in a way prettier's own line width does not, which lint:ci caught as a formatting warning.
|
Both blockers were right, fixed on top at The over-length name. You were right that The eager "not on the mod database" note. You were right that folding a transport failure into the same bucket as a 404 turns an outage into a false claim about forks and private builds. Fixed by having Non-blocking: |
|
Correction to the line above: the head did not move, |
Zaldaryon
left a comment
There was a problem hiding this comment.
Re-review, head 677bb06.
Both blockers are fixed. I ran the full local gate on 677bb06 and it is green: typecheck, lint:ci (0 errors; the 15 hook warnings are all pre-existing and none are in the files this PR touches), format:check, test:coverage (2195 passed, 2 skipped, 93.57% statements and 90.37% branches against the 87 and 85 floors), and build:unpack. Every required CI context passes on this head.
The over-length name. clampModpackModName in src/domain/mods/importModpack.ts cuts to MAX_MODPACK_MOD_NAME_LENGTH, and both the writer (toModpackManifest) and the reader (parseModpackManifest) now read that one constant, so they cannot drift. The boundary is right: assertString rejects only length > maxLength and length === 0, a name at exactly 256 passes both directions, and the clamp can never return an empty string because it only runs on input longer than the cap. The surrogate guard holds, a prefix cut can only orphan a high surrogate at the tail and the regex drops it. useExportModpack is the only caller of toModpackManifest and both the plain and the (Server) export buttons route through it, so nothing sends an unclamped name into the reader anymore. A hand-crafted file with a 300-character name is still a clean refusal through the existing try/catch, not a crash. Tests cover export at the cap, export one character over the cap refused, and the export-then-import round trip.
The eager "not on the mod database" note. useQueryMod now returns found | not-found | failed, and only a real 404 (api-error with statusCode "404", which is how the v1 envelope reports a missing mod) counts as not-found. A thrown transport error, a non-2xx HTTP status, and a payload that does not parse all land on failed. planModpackImport takes the failed set and gives those entries their own lookup-failed reason, the popup renders it as "Couldn't reach the mod database" with a retry, and a batch where every lookup failed replaces the table with the unreachable panel instead of a page of wrong "not on the mod database" rows. The fix does not flip the bug the other way: a genuine 404 still reads as not-found. The retry effect resets details and failedModids before it re-runs, so a retry shows the checking state again, and the close-mid-lookup path is still covered by its test. The two other useQueryMod callers keep their old behaviour; folding not-found and failed together is all a compatibility scan or a single-release catalog has ever needed.
Non-blocking, for a follow-up:
Opening a second, different pack in the same session without the popup unmounting renders one frame of the new pack's rows scored against the previous pack's details and failedModids, with Import enabled on that stale plan, before the effect resets and re-resolves. useModReleaseCatalog handles the same shape by resetting in the !manifest branch rather than bailing out of it. modStatuses has the same one-frame staleness and predates this PR.
The retry button and the allLookupsFailed panel have no !importing guard, so a retry click during an import starts a fresh round of lookups that cannot change the plan already running, and if the database is down by then it swaps the table and the progress bar for the unreachable panel while downloads continue. Gate both on !importing.
allLookupsFailed is computed over modpackEntriesToResolve, which excludes entries already satisfied on disk, so a pack that is 39 mods already installed and 1 mod a failed lookup shows "none of these mods could be checked yet" and hides the 39. Wording the panel in terms of the entries that needed a lookup, or requiring the whole manifest to be unresolved, would fix it.
A lookup-failed row falls through statusColor and StatusIcon to the red "prohibited" treatment, the same as not-on-moddb, while the note right below it is orange with a warning icon. It reads as a hard failure in the table and a transient one everywhere else. text-red-400 on the popup table row is also still unpinned in tests/text-contrast.test.ts, the same gap that shipped text-red-700 at 2.18:1.
malformed-response folds into failed silently. A page that exists but whose body does not parse now offers a retry that cannot succeed and logs nothing, while useQueryMods logs the reason on the same failure. One debug line on the !parsed.ok branch would make it diagnosable.
The QUERY_URL limiter is one 6-wide lane shared by every caller of the channel, so a 200-entry manifest queues the browse-mods catalog and the tag and version fetches behind up to 200 detail lookups. The bound is the right call. A per-path-kind lane, or one reserved slot for non-/mod/ paths, would keep the other screens responsive. This overlaps #386.
Approving. The non-blocking items are worth a follow-up, none of them block this.
NekoJess exported a 200-mod pack for a friend and read an import table of nothing but modids, which is why she thought the launcher was pulling in a mod she had never installed. It was not: all 202 ids probe clean, 199 resolve to themselves and 3 are not on the database at all, but a table of
th3dungeon,terrapretyandthetruearchimedesscrewgives a player no way to see that, and the table said nothing about what each row was going to do to her folder until after it had done it.Five commits, in order.
The export carries the local name. Each manifest entry now writes the display name from the installed copy's
modinfo.jsonnext to the modid and the version. The reader takes a manifest with names and one without, because every pack exported until now has none. The name is a label only, the modid is still the identifier, and building the manifest moved from the export hook into the mods adapter so a test can pin the shape.The table labels each row with the best name it has.
modpackRowLabeltakes the mod database name once the lookup resolved it, then the name the pack was exported with, then the modid. A plan item that named itself after its own modid counts as unnamed, which is what makes the three unresolvable rows fall through to their local names instead of repeating the id twice. The modid stays as a second line whenever the label is not the modid itself, and the row key is still the modid.The plan is visible before Import is clicked.
planModpackImportalready decided all of it, somodpackRowStatusreads a plan item back as the sentence its row shows: new install, update from one version to another, downgrade, already installed, not on the mod database, no release. The lookups moved from the Import click to the moment the manifest opens, since they cost the same either way and the import needs them regardless. Import waits on them and says so. Once an entry starts moving, what happened outranks what was going to happen, so the live status wins the row back.The "not on the mod database" wording says what was checked. Her three ids resolve to nothing because the hub's Alloy Calculator listing declares
alloycalculatorwhile her copy declaresalloycalculatorstuzzichino. The old "Not found on ModDB" read as if the mod did not exist, which is what sent her looking for it. A note under the table now says no listing there declares the mod id in any of its releases, so those copies are most likely forks or private builds and have to be installed by hand.A copy the pack does not accept says so before it is replaced.
satisfyingCopyaccepts an installed copy only when it is enabled and at the exact version the manifest names, so a mod whose version string was edited by hand, or one the player had turned off, is re-downloaded over the top. Nothing changes about what gets installed. The row says the installed copy will be replaced and names both versions.When the lookups run, and how many at once
Worth calling out on its own because it changes when the launcher talks to the mod database, not just what the table says.
Before this branch, the detail lookups happened on the Import click. They now happen when the manifest loads. That is the whole reason the table can name a mod and say what will happen to it before the player commits to anything: the plan needs those lookups, and running them on open costs exactly what running them on click cost. Only the entries the installed folder does not already satisfy are asked about, same as before.
What that changed is who triggers the fan-out. A 200-mod pack used to send 200 requests at the mod database when the player pressed a button; it now sends them off a file chooser. Nothing downstream bounded that.
pathsHandlers.tslanes downloads and archive work throughConcurrencyLimiter, butQUERY_URLhad no ceiling of any kind, so the burst went out as one open socket per mod.QUERY_URLnow runs its request inside a six-slot limiter in the main process, declared and shut down the same way the download and archive lanes are, withbefore-quitwired to it alongside them. Six is the per-host connection cap Chrome and Firefox have used for years, so a burst from the launcher is no heavier on a community-run site than an ordinary visit to one of its pages, and 200 lookups in lanes of six is 34 rounds, seconds rather than minutes. The bound sits in the IPC layer rather than in the popup because every caller of the channel shares the one remote host and only the main process sees all of them at once: Manage Mods resolves one detail per installed mod and fans out the same way.tests/ipc/netHandlersDispatch.test.tspins it through the real handler with the transport stubbed. Forty requests go in, every one held open until the peak has been read, and the peak is asserted to be six with all forty answered. The expected peak is written out as a literal rather than read back from the constant, so raising the bound in the source fails the test instead of moving it.A queued lookup looks to the renderer exactly like a slow one, which is what the "Checking the mod database..." state already renders, and Import stays disabled until the plan exists. Queueing cannot trip a request's own timeout either:
requestBoundedTextstarts its wall clock inside the task, and the limiter does not run the task until a slot is free. The effect that fires the lookups cancels on cleanup, so a batch that comes back after the popup was closed and re-opened on another pack is dropped instead of landing on the new table.One row, before and after:
Review fixes
Zaldaryon's review on
0b661b6caught two real gaps, both about a case this branch newly reaches. Four commits on top fix them and the two non-blocking follow-ups.A mod name over 256 characters no longer fails Export Modpack.
toModpackManifestcopiedmod.nameverbatim, the scanner reads amodinfo.jsonname up to 4096 characters, andparseModpackManifest'sassertString(entry.name, "mod name", 256)refused anything past 256, so one long-named installed mod failed the whole export. A label must never be the reason an export fails, sotoModpackManifestnow clamps the name through a new domain helper,clampModpackModName, cutting to the cap on a UTF-16 boundary that never splits a surrogate pair. The cap itself is one constant,MAX_MODPACK_MOD_NAME_LENGTH, read by both the writer and the reader, so the two cannot drift apart again. Covered in the domain (the clamp, including the surrogate-pair boundary), in the adapter (an over-length name comes out clamped), and at the IPC handler (a name at exactly the cap exports and re-imports cleanly).A lookup that never answered no longer reads as "not on the mod database".
useQueryModcaught every failure alike and returnedundefined, so a transport failure was indistinguishable from a clean 404. With the database unreachable, every unresolved entry becamenot-on-moddb, and the note under the table told the player, before they had clicked anything, that those mods were forks or private builds to install by hand.useQueryModnow returns a discriminated outcome (found/not-found/failed), a 404 read offparseModDetailResponse's own envelope check is the only thing that counts asnot-found, and everything else (a thrown network error, a timeout, an envelope that does not check out as a real 404) isfailed.planModpackImporttakes an optional set of modids whose lookup failed and gives those a new skip reason,lookup-failed, kept apart fromnot-on-moddb. The import popup renders it as "Couldn't reach the mod database" with its own note and a retry action, and swaps the whole table for an unreachable state, retry included, when every lookup in the batch failed. The two other callers ofuseQueryMod(the installed-mods update scan, the single-release catalog) still fold not-found and failed together, which is all they have ever needed.Non-blocking, both done.
text-orange-300andtext-blue-400(the downgrade/replace and downloading/update row hues) are now pinned intests/text-contrast.test.tsthe way #366 pins the release-table verdicts, computing the ratio through the samepaletteForeground/tailwindColormachinery rather than quoting Zaldaryon's measured 9.47:1 and 6.28:1. And #386 tracks his other note, quoting his reasoning: the single globalQUERY_URLFIFO means Manage Mods' still-unbounded installed-mod fan-out can make a later catalog or filter fetch wait behind a full page scan, so bounding that fan-out at its own call site would keep one page's scan from starving another page's first request.Mutants
Each applied alone against the committed tree, tests run, tree restored and checked clean before the next.
modpackRowLabelreturns the modid instead of the local namemodpackRowStatusreturns "new" where it returns "update"QUERY_URLcalls the transport baretoModpackManifestcopies it verbatim again)not-on-moddbinuseQueryModtext-orange-300,text-blue-400) is regressed totext-red-700Testing
On head
677bb06:npm run typecheck: passed (node, web, tests).npm run lint:ci: 0 errors, 15 pre-existing React Hooks warnings.npm run format:check: passed.npm run test:coverage: 169 files, 2195 passed, 2 skipped; 93.57% statements, 90.37% branches, 93.25% functions, 95.15% lines locally, floors 87 / 85 / 85 / 89.npm run build:unpack: passed, Electron 44.1.1 Linux.npx vitest run tests/domain/mods/importModpack.test.ts: 43 passed.npx vitest run tests/renderer-dom/importModpackPopup.test.tsx: 15 passed, one row of every kind read before Import is clicked, the close/re-open mid-lookup, and the mixed/all-failed/retry lookup-failure cases.npx vitest run tests/ipc/modsHandlers.test.ts: 20 passed, including the name-clamp boundary on export and import.npx vitest run tests/renderer/importModpackAdapter.test.ts: 4 passed, including the clamp.npx vitest run tests/text-contrast.test.ts: 21 passed, including the two new hue pins.npx vitest run tests/ipc/netHandlersDispatch.test.ts: 5 passed, including the concurrency bound.Related issues
Closes #379
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.