diff --git a/src/renderer/src/extensions/collections/util/InstallDriver.test.ts b/src/renderer/src/extensions/collections/util/InstallDriver.test.ts index ffb54802ad..518c0481ae 100644 --- a/src/renderer/src/extensions/collections/util/InstallDriver.test.ts +++ b/src/renderer/src/extensions/collections/util/InstallDriver.test.ts @@ -16,6 +16,7 @@ import { describe, expect, vi } from "vitest"; import { makeCollectionModInfo, + makeDeterministicRef, makeDownload, makeFileListItem, makeInstallerChoices, @@ -212,6 +213,79 @@ describe("InstallDriver optional default-skip", () => { expect(rules.find((r) => r.reference.tag === "req-a")?.ignored).toBeUndefined(); }); + // Nexus file names repeat across unrelated mods ("Main File"), so a required member listed before + // an optional can share its logical file name. Defaulting the optional must skip the optional, + // not the required member, which would then never be installed, with the collection reported + // complete. + test("defaults the optional, not a required member with the same file name", async ({ + makeDriver, + }) => { + const sharedReq = makeRule({ + type: "requires", + reference: makeReference({ tag: "req-main", logicalFileName: "Main File" }), + }); + const sharedOpt = makeRule({ + type: "recommends", + reference: makeReference({ tag: "opt-main", logicalFileName: "Main File" }), + }); + const fixture = buildCollectionFixture("col-shared", [sharedReq, sharedOpt]); + const h = makeDriver({ + mods: { [GAME_ID]: { [fixture.collection.id]: fixture.collection } }, + downloads: { [fixture.download.id]: fixture.download }, + profiles: { [profile.id]: profile }, + }); + await startWith(h, fixture); + + const session = h.getState().session.collections.activeSession; + expect(session?.mods[modRuleId(sharedOpt)].status).toBe("ignored"); + expect(session?.mods[modRuleId(sharedReq)].status).not.toBe("ignored"); + const rules = h.getState().persistent.mods[GAME_ID]["col-shared"].rules ?? []; + expect(rules.find((r) => r.reference.tag === "opt-main")?.ignored).toBe(true); + expect(rules.find((r) => r.reference.tag === "req-main")?.ignored).toBeUndefined(); + }); + + // A deterministic collection tags a fuzzy member by its mod page and install spec, so a required + // and an optional file from one page share a tag. Defaulting the optional must still skip it, + // not the required member. + test("defaults the optional, not a required member sharing its deterministic tag", async ({ + makeDriver, + }) => { + const page = { repository: "nexus", gameId: GAME_ID, modId: "1234", fileId: "1" }; + const sharedReq = makeRule({ + type: "requires", + reference: makeDeterministicRef({ + repo: page, + versionMatch: ">=1.0.0+prefer", + logicalFileName: "Main File", + }), + }); + const sharedOpt = makeRule({ + type: "recommends", + reference: makeDeterministicRef({ + repo: { ...page, fileId: "2" }, + versionMatch: ">=1.0.0+prefer", + logicalFileName: "Optional patch", + }), + }); + // the fixture models the real rule: both members carry the same tag + expect(sharedReq.reference.tag).toBeDefined(); + expect(sharedOpt.reference.tag).toBe(sharedReq.reference.tag); + const fixture = buildCollectionFixture("col-det", [sharedReq, sharedOpt]); + const h = makeDriver({ + mods: { [GAME_ID]: { [fixture.collection.id]: fixture.collection } }, + downloads: { [fixture.download.id]: fixture.download }, + profiles: { [profile.id]: profile }, + }); + await startWith(h, fixture); + + const session = h.getState().session.collections.activeSession; + expect(session?.mods[modRuleId(sharedOpt)].status).toBe("ignored"); + expect(session?.mods[modRuleId(sharedReq)].status).not.toBe("ignored"); + const rules = h.getState().persistent.mods[GAME_ID]["col-det"].rules ?? []; + expect(rules.find((r) => r.type === "recommends")?.ignored).toBe(true); + expect(rules.find((r) => r.type === "requires")?.ignored).toBeUndefined(); + }); + test("does not re-default an optional the user already selected (ignored:false)", async ({ makeDriver, }) => { diff --git a/src/renderer/src/extensions/collections/util/InstallDriver.ts b/src/renderer/src/extensions/collections/util/InstallDriver.ts index b4e2e2a333..2a3ac848b9 100644 --- a/src/renderer/src/extensions/collections/util/InstallDriver.ts +++ b/src/renderer/src/extensions/collections/util/InstallDriver.ts @@ -12,6 +12,7 @@ import type { IState } from "../../../types/IState"; import { generateCollectionSessionId, isTerminalMemberStatus, + modRuleId, } from "../../../util/collectionInstallSession"; import { getCollectionActiveSession, @@ -997,7 +998,10 @@ class InstallDriver { // rule flag + session status) now that the session exists. for (const rule of optional) { if (rule.ignored === undefined) { - markCollectionMemberSkipped(this.mApi, { reference: rule.reference }); + markCollectionMemberSkipped(this.mApi, { + reference: rule.reference, + ruleId: modRuleId(rule), + }); } } diff --git a/src/renderer/src/extensions/mod_management/InstallManager.ts b/src/renderer/src/extensions/mod_management/InstallManager.ts index 1abd635dcf..4318532334 100644 --- a/src/renderer/src/extensions/mod_management/InstallManager.ts +++ b/src/renderer/src/extensions/mod_management/InstallManager.ts @@ -857,6 +857,22 @@ class InstallManager { this.maybeAdvancePhase(collectionId, api); } + /** + * Whether a dependency is a member of the collection being installed: it carries the member's + * session key, or it belongs to the collection itself rather than to one of its members. A + * member's own dependencies are installed with the member as their source and are not members. + */ + private isCollectionMemberDependency( + api: IExtensionApi, + sourceModId: string, + dep: IDependency, + ): boolean { + return ( + dep.sessionRuleId !== undefined || + getCollectionActiveSession(api.getState())?.collectionId === sourceModId + ); + } + private handleDownloadSkipped(api: IExtensionApi, sourceModId: string, dep: IDependency) { if (!sourceModId || !dep) { return; @@ -882,8 +898,11 @@ class InstallManager { } // Mark the skipped member ignored directly against the active session (collections is core - // now, so no event round-trip through the InstallDriver is needed). - markCollectionMemberSkipped(api, { reference: dep.reference }); + // now, so no event round-trip through the InstallDriver is needed). A sub-dependency of a + // member is not a member itself, so its skip ignores nothing. + if (this.isCollectionMemberDependency(api, sourceModId, dep)) { + markCollectionMemberSkipped(api, { reference: dep.reference, ruleId: dep.sessionRuleId }); + } // See if we can advance the phase this.maybeAdvancePhase(sourceModId, api); @@ -2720,8 +2739,14 @@ class InstallManager { } else { this.mDependencyRetryCount.delete(installKey); if (recovery.action === "skip") { - // same settle the free-user skip performs: durable ignore + session "ignored" - markCollectionMemberSkipped(api, { reference: dep.reference }); + // same settle the free-user skip performs: durable ignore + session "ignored". + // A sub-dependency of a member is not a member itself, so its skip ignores nothing. + if (this.isCollectionMemberDependency(api, sourceModId, dep)) { + markCollectionMemberSkipped(api, { + reference: dep.reference, + ruleId: dep.sessionRuleId, + }); + } } else if (recovery.action === "fail") { // Retries exhausted: settle the member as failed (terminal) so the collection can // still complete and the member is not re-prompted. writeCollectionSession no-ops diff --git a/src/renderer/src/test-utils/builders.ts b/src/renderer/src/test-utils/builders.ts index feac92ed87..b3325eb7e8 100644 --- a/src/renderer/src/test-utils/builders.ts +++ b/src/renderer/src/test-utils/builders.ts @@ -36,6 +36,7 @@ import type { ICollectionMod, ICollectionModRule, } from "../extensions/collections/types/ICollection"; +import { deterministicReferenceTag } from "../extensions/collections/util/deterministicReferenceTag"; import type InstallDriver from "../extensions/collections/util/InstallDriver"; import { stateReducer as downloadStateReducer } from "../extensions/download_management/reducers/state"; import { downloadPathForGame } from "../extensions/download_management/selectors"; @@ -135,6 +136,14 @@ export function makeFuzzyRef(overrides: Partial = {}): IModRefere return makeExactRef({ versionMatch: "*", ...overrides }); } +// A reference tagged the way a deterministic collection tags its members (transformCollection): +// the tag is derived from the reference, so two fuzzy files from one mod page share it, as they do +// in real collections. Tests of member identity must use this rather than a hand-picked tag. +export function makeDeterministicRef(overrides: Partial = {}): IModReference { + const reference = makeExactRef({ tag: undefined, ...overrides }); + return { ...reference, tag: deterministicReferenceTag(reference) }; +} + export function makeRule(overrides: Partial = {}): IModRule { return { type: "requires", diff --git a/src/renderer/src/util/collectionSkip.test.ts b/src/renderer/src/util/collectionSkip.test.ts index e34557b051..541c0f7c8b 100644 --- a/src/renderer/src/util/collectionSkip.test.ts +++ b/src/renderer/src/util/collectionSkip.test.ts @@ -10,6 +10,7 @@ import { describe, expect } from "vitest"; import type { IModRule } from "../extensions/mod_management/types/IMod"; import { + makeDeterministicRef, makeInstallState, makeMod, makeModInstallInfo, @@ -141,6 +142,123 @@ describe("markCollectionMemberSkipped - automatic skip (mod reference)", () => { expect(flagged).toEqual(["tag-x"]); }); + // the member the tag names wins over one met earlier that only shares a weaker marker, in the + // session scan and in the live-rule scan alike + test.for([ + ["file hash", { fileMD5: "shared-md5" }], + ["logical file name", { logicalFileName: "Main File" }], + ] as const)( + "ignores the tagged member, not an earlier one with the same %s", + ([, shared], { makeApi }) => { + const earlier = makeRule({ + type: "requires", + reference: makeReference({ tag: "tag-earlier", ...shared }), + }); + const skipped = makeRule({ + type: "recommends", + reference: makeReference({ tag: "tag-skipped", ...shared }), + }); + const h = makeApi({ + mods: { + [GAME_ID]: { + [COLLECTION_ID]: makeMod({ id: COLLECTION_ID, rules: [earlier, skipped] }), + }, + }, + session: makeInstallState({ + activeSession: makeSession({ + sessionId: SESSION_ID, + collectionId: COLLECTION_ID, + gameId: GAME_ID, + mods: { + [modRuleId(earlier)]: makeModInstallInfo({ rule: earlier, status: "pending" }), + [modRuleId(skipped)]: makeModInstallInfo({ rule: skipped, status: "pending" }), + }, + }), + }), + }); + + const matched = markCollectionMemberSkipped(h.api, { reference: skipped.reference }); + + expect(matched).toBe(true); + expect(statusOf(h, skipped)).toBe("ignored"); + expect(statusOf(h, earlier)).toBe("pending"); + const rules = h.getState().persistent.mods[GAME_ID][COLLECTION_ID].rules ?? []; + const flagged = rules + .filter((rule) => rule.ignored === true) + .map((rule) => rule.reference.tag); + expect(flagged).toEqual(["tag-skipped"]); + }, + ); + + // two fuzzy files from one mod page share a deterministic tag; the skip site's rule id tells + // them apart where no marker can + test("ignores the member its rule id names when another shares its deterministic tag", ({ + makeApi, + }) => { + const page = { repository: "nexus", gameId: GAME_ID, modId: "1234", fileId: "1" }; + const required = makeRule({ + type: "requires", + reference: makeDeterministicRef({ repo: page, versionMatch: ">=1.0.0+prefer" }), + }); + const optional = makeRule({ + type: "recommends", + reference: makeDeterministicRef({ + repo: { ...page, fileId: "2" }, + versionMatch: ">=1.0.0+prefer", + }), + }); + expect(optional.reference.tag).toBe(required.reference.tag); + const h = makeApi({ + mods: { + [GAME_ID]: { + [COLLECTION_ID]: makeMod({ id: COLLECTION_ID, rules: [required, optional] }), + }, + }, + session: makeInstallState({ + activeSession: makeSession({ + sessionId: SESSION_ID, + collectionId: COLLECTION_ID, + gameId: GAME_ID, + mods: { + [modRuleId(required)]: makeModInstallInfo({ rule: required, status: "pending" }), + [modRuleId(optional)]: makeModInstallInfo({ rule: optional, status: "pending" }), + }, + }), + }), + }); + + const matched = markCollectionMemberSkipped(h.api, { + reference: optional.reference, + ruleId: modRuleId(optional), + }); + + expect(matched).toBe(true); + expect(statusOf(h, optional)).toBe("ignored"); + expect(statusOf(h, required)).toBe("pending"); + const rules = h.getState().persistent.mods[GAME_ID][COLLECTION_ID].rules ?? []; + expect(rules.filter((rule) => rule.ignored === true).map((rule) => rule.type)).toEqual([ + "recommends", + ]); + }); + + // a tagged skip that names no member (a sub-dependency, say) must not land on a member that + // only shares its logical file name + test("does not fall back to the file name for a skip that carries a tag", ({ makeApi }) => { + const rule = makeRule({ + type: "requires", + reference: makeReference({ tag: "mod-a", logicalFileName: "Main File" }), + }); + const h = makeApi(ruleOverrides(rule)); + + const matched = markCollectionMemberSkipped(h.api, { + reference: makeReference({ tag: "not-a-member", logicalFileName: "Main File" }), + }); + + expect(matched).toBe(false); + expect(statusOf(h, rule)).toBe("pending"); + expect(durableIgnored(h)).toBeUndefined(); + }); + // a session can track a member the collection's current rules no longer carry (the rules were // replaced mid-install); the skip settles the session entry without re-adding the old rule test("settles a member whose rule left the collection without re-adding it", ({ makeApi }) => { diff --git a/src/renderer/src/util/collectionSkip.ts b/src/renderer/src/util/collectionSkip.ts index 711b9a3fd9..a35d99cea0 100644 --- a/src/renderer/src/util/collectionSkip.ts +++ b/src/renderer/src/util/collectionSkip.ts @@ -28,8 +28,9 @@ export type ISkippedDownloadIdentifiers = Omit fileName.toLowerCase().replace(/[^a-z]+/gi, ""); @@ -61,28 +62,45 @@ function fuzzyChainMatch(identifiers: ISkippedDownloadIdentifiers, ref: IModRefe } /** - * Match the skipped dependency's reference against a collection rule. This mirrors the previous - * `collection-mod-skipped` handler exactly (tag is most reliable, then file hash, then logical - * file name) so the automatic/premium skip path is behaviourally unchanged. + * The identity markers a skip without a rule id is matched on, strongest first. None of them is + * unique: a deterministic tag is shared by fuzzy files from one mod page, a file hash by members + * installing the same archive, and a logical file name ("Main File") by unrelated files. So a skip + * carrying a tag never falls back to the file name, and a skip that can name its member's rule + * (`ruleId`) is not matched on markers at all. */ -function matchesReference(reference: IModReference, ruleRef: IModReference): boolean { - if (reference.tag && ruleRef.tag === reference.tag) { - return true; - } - if (reference.fileMD5 && ruleRef.fileMD5 === reference.fileMD5) { - return true; - } - if (reference.logicalFileName && ruleRef.logicalFileName === reference.logicalFileName) { - return true; - } - return false; -} +const markersFor = (reference: IModReference) => + reference.tag ? (["tag", "fileMD5"] as const) : (["fileMD5", "logicalFileName"] as const); -function matchesSkip(skip: ICollectionSkip, ref: IModReference): boolean { - if ("reference" in skip) { - return matchesReference(skip.reference, ref); +/** + * Find the item a skip names by its markers. Each marker is tried against every item before the + * next, weaker one, so an earlier member that only shares a weaker marker is never taken for the + * one the stronger marker names. + */ +function findSkipped( + skip: ICollectionSkip, + items: T[], + referenceOf: (item: T) => IModReference | undefined, +): T | undefined { + if (!("reference" in skip)) { + return items.find((item) => { + const ref = referenceOf(item); + return ( + ref != null && + (testRefByIdentifiers(skip.identifiers, ref) || fuzzyChainMatch(skip.identifiers, ref)) + ); + }); + } + for (const marker of markersFor(skip.reference)) { + const value = skip.reference[marker]; + if (!value) { + continue; + } + const found = items.find((item) => referenceOf(item)?.[marker] === value); + if (found !== undefined) { + return found; + } } - return testRefByIdentifiers(skip.identifiers, ref) || fuzzyChainMatch(skip.identifiers, ref); + return undefined; } /** @@ -116,18 +134,22 @@ export function markCollectionMemberSkipped(api: IExtensionApi, skip: ICollectio ); // The session is keyed by each member's rule as it was when the install started, so the entry // is matched on its own snapshot rather than on the live rule. + const namedRuleId = + "ruleId" in skip && skip.ruleId !== undefined && session.mods[skip.ruleId] !== undefined + ? skip.ruleId + : undefined; const [sessionRuleId] = - Object.entries(session.mods).find(([, info]) => - info.rule?.reference != null ? matchesSkip(skip, info.rule.reference) : false, - ) ?? []; + namedRuleId !== undefined + ? [namedRuleId] + : (findSkipped(skip, Object.entries(session.mods), ([, info]) => info.rule?.reference) ?? []); - // The fallback scan relies on members having distinct identities; if two share the matched - // identifier (e.g. the same logicalFileName, or two fuzzy rules on one modId), the wrong member - // could be ignored. + // The marker scan relies on members having distinct identities; if two share the matched + // marker (two fuzzy files from one mod page share a deterministic tag), the wrong member could + // be ignored. Skip sites that have the member's rule pass its ruleId to avoid the scan. const rule = (sessionRuleId !== undefined ? rules.find((iter) => modRuleId(iter) === sessionRuleId) - : undefined) ?? rules.find((iter) => matchesSkip(skip, iter.reference)); + : undefined) ?? findSkipped(skip, rules, (iter) => iter.reference); // Only a rule the session tracks can be settled. A live rule it never saw (the collection // gained it after the install started) still carries the decision durably. const liveRuleId = rule !== undefined ? modRuleId(rule) : undefined;