Skip to content

Suggest compatible Mods for an Installation - #498

Open
Zaldaryon wants to merge 6 commits into
devfrom
fix/issue-464-suggestions
Open

Zaldaryon wants to merge 6 commits into
devfrom
fix/issue-464-suggestions

Conversation

@Zaldaryon

@Zaldaryon Zaldaryon commented Sep 15, 2026

Copy link
Copy Markdown
Collaborator

Summary

Adds an opt-in suggestions row to the Mods browse page for compatible Mods the selected Installation does not have. Suggestions are ranked locally from the existing ModDB catalog and explain the signal that caused each card to appear.

Type

  • Bug fix
  • Feature
  • Performance
  • Refactor or cleanup
  • Tests only
  • Docs or build

Checklist

  • Targets dev, not main.
  • npm run typecheck passes.
  • npm run lint:ci passes with the repository's existing warnings and no errors.
  • npm run format:check passes.
  • npm run test:coverage passes, coverage at or above the floor in vitest.config.ts.
  • npm run build:unpack passes.

Testing

  • Focused feature, config, renderer, and i18n suites: 246 tests passed.
  • The ranker tests cover filtering, exclusions, signal explanations, compatible copies from other Installations, logarithmic popularity, recency, detail lookup limits, cancellation, and undeclared compatibility.
  • The DOM tests cover opt-in network isolation, rendering and reasons, refresh, dismissal persistence, and Add all opening the existing confirmation dialog without a download.
  • Full npm run test:coverage was attempted locally. This workspace has Node v26.7.0 while CI uses Node 22; the run hit existing LZMA, session, and unrelated renderer timeout failures. The focused feature suites passed.
  • npm run build:unpack passed on Linux.

Related issues

Fixes #464

@Zaldaryon
Zaldaryon requested a review from Pixnop September 15, 2026 12:40
@Zaldaryon
Zaldaryon marked this pull request as ready for review September 15, 2026 18:39

@Zaldaryon Zaldaryon left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Withdrawn]

@Zaldaryon Zaldaryon mentioned this pull request Sep 15, 2026
12 tasks

@Pixnop Pixnop left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ran the full gate on Node 22 to match CI, since the coverage box in the checklist was left unchecked: typecheck clean, lint 0 errors with the same 14 pre-existing warnings and none of them in the new files, and a full vitest run at 236 files, 4456 tests passed, 2 skipped, nothing red. That clears the Node 26 caveat directly, the LZMA and session failures in the description do not reproduce on 22, so the unchecked box should not hold this up. Mutation testing against the ranker and the config normalizer killed every mutant tied to something #464 or a test actually describes: the other-installation guard, all four exclusion filters, the score and tie-break sort, both resolveSuggestions caps, the undeclared-release filter, both cancellation checks. Built and ran the packaged Linux build headless on a seeded two-Installation profile: the row stays hidden and silent before opt-in, the six ranked cards and their reasons match the domain logic, dismissal survives a full restart, Add all opens the existing install dialog and cancelling leaves the Mods folder untouched, and the whole row is reachable and operable by keyboard. Boundary holds too, no window.api anywhere under features/mods, and log-provenance, i18n-parity and security-boundaries all pass as is.

One thing needs fixing before this merges. The suggestions heading block has nothing under it but the shell scrim, and the caption at ModSuggestions.tsx:90 (text-xs text-zinc-400) comes out at 3.01:1 against a light background image, redoing tests/text-contrast.test.ts's own math, below the 4.5:1 floor that file holds every other piece of body text to. Load a light background image, open Mods with the suggestions row showing, and the caption under the heading is close to unreadable. That suite passes in full, 213 assertions, precisely because nothing in it points at this component yet. The card footer text right next to it does not have this problem, it sits inside the grid panel's own scrim and lands at 8.25:1. Give the caption the same panel or move it inside GridWrapper, and add the missing assertReadable line so this stays caught going forward.

Should-fix, not a blocker: dismissing one suggestion re-runs the whole pipeline, a fresh catalog fetch, a fresh scan of every other Installation, fresh detail lookups, because dismissedListingIds sits in the effect's dependency array at useModSuggestions.ts:113 and ADD_DISMISSED_MOD_SUGGESTION always hands back a new array. Seed two Installations, opt in, click dismiss on one card, and watch a second GET_INSTALLED_MODS scan and a visible reload fire for what should be an instant local removal. The already-ranked candidates past the six shown could backfill the card for free. #464 itself argues for going easy on ModDB's server, this goes the other way on every dismiss, worth a follow-up.

Also should-fix: normalizeConfig's own handling of the two new fields is not pinned anywhere. Force normalizeModSuggestionsConsent (configManager.ts:429) to always return null, or normalizeDismissedModSuggestions (configManager.ts:433) to always return an empty array, and every test in configManager.test.ts and migrations.test.ts still passes. favMods and suspendedModUpdates get this coverage directly through normalizeConfig in the same file, these two should too.

A few notes worth knowing rather than acting on. The schema bump to 6 and its migration duplicate what normalizeConfig already computes unconditionally for these two fields, cutting against the "must not need a schema bump" precedent this same test file states for accentColor and lastSeenChangelogVersion, but #464 explicitly asked for the bump and a cap on the dismissed list, so call it intentional rather than a slip. The catch around a failing detail lookup in suggestions.ts (line 195) is real but nothing exercises a throwing getDetail, same for the category-tag cap and case-insensitive tag matching, none of the fixtures use more than one tag. Enter did not trigger the buttons in the headless build where Space did, that reads as an artifact of the test environment rather than anything this PR did.

@github-actions

github-actions Bot commented Sep 15, 2026

Copy link
Copy Markdown

Translation status

en-US is the source and carries 889 keys.

Locale Keys Missing Stale Drafted to review
be-BY 882 15 8 645
de-DE 874 15 0 725
es-ES 878 15 4 532
fr-FR 893 0 4 0
hu-HU 874 15 0 600
it-IT 878 15 4 539
nl-NL 874 15 0 725
pl-PL 882 15 8 536
pt-BR 878 15 4 475
pt-PT 878 15 4 549
ru-RU 882 15 8 633
uk-UA 882 15 8 558
zh-CN 874 15 0 715

The status page is out of date. Refresh it with npm run i18n:status -- --write docs/contribute/translation-status.md.

Drafted values are the machine-drafted ones still waiting for a native review, listed per locale in src/renderer/src/locales/drafted.json. See #496.

@Zaldaryon
Zaldaryon requested a review from Pixnop September 15, 2026 23:15
@Zaldaryon

Copy link
Copy Markdown
Collaborator Author

Implemented Pixnop's requested changes on this head.

  • The Mod suggestions heading and caption now share the readable grid panel.
  • Dismissal removes cards locally without rerunning the suggestion pipeline, while the detail lookup budget backfills the visible row.
  • Config normalization coverage now covers consent and dismissed suggestion IDs.

Validation passed locally:

  • Focused suites: 163 tests across 4 files.
  • npm run typecheck
  • npm run lint:ci with 0 errors and 14 pre-existing warnings
  • npm run format:check
  • npm run build:unpack

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 Pixnop left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two of the three requests are met, one is not.

What holds. Dismissal is local: dismissedListingIds moved to a ref (useModSuggestions.ts:43-44) and out of the deps (:116), and both halves die under mutation against tests/renderer-dom/modsSuggestions.test.tsx:165. Live, dismissing a card vanished at once, backfilled, fired no GET_INSTALLED_MODS and no new ModDB request, and survived a relaunch. normalizeConfig is pinned: forcing either normalizer to a constant fails tests/ipc/configManager.test.ts:188 and :197. 105 rule tests and 239 feature tests pass, 22 of 34 mutants die, the mods folder names no window.api, the two new log lines carry only allowed tokens. Opt-in gating, the schema 5 to 6 migration, Add all and keyboard operation check out on the packaged build.

Blocking.

  1. ModSuggestions.tsx:86. The heading row moved inside GridWrapper but has no positioning, so the wrapper's before: scrim (Grid.tsx:20, backdrop-blur-sm) paints over it while GridGroup and FormButton, both relative, escape. Repro: build the renderer and serve ModSuggestions.tsx:84-103 against the compiled stylesheet. The caption reads 1.18:1, worse than the 3.01:1 I flagged; adding relative to the :86 div lifts it to 7.09:1. tests/text-contrast.test.ts:386-393 greps class names, so it stays green.

  2. useModSuggestions.ts:116. The deps take installations and installedMods by identity, and syncModsCount rewrites those objects on every scan (ListMods.tsx:205 keys on id and path for that reason). Repro: mount with consent true and a 30 entry catalog, the effect runs twice: 2 catalog downloads and 40 detail requests for 6 cards.

Should fix.

  1. useModSuggestions.ts:95 pays for the backfill up front, 20 lookups per load and per Refresh instead of 6, and nothing pins the value.

  2. suggestions.ts:124 picks the reason from the largest raw signal, not the largest weighted term, so a card can name a term contributing 4.00 against another's 10.67. Zeroing the trending or popularity weight leaves the suite green.

  3. tests/text-contrast.test.ts:389 uses foreground, not foregrounds, so only the caption is pinned; :72 and :119 go to zinc-500 and zinc-600 with 48 tests passing.

  4. tests/renderer/configReducer.test.ts:121 seeds the list at the cap, so both dispatches hit the length guard and the includes check at configReducer.ts:407 deletes clean with the suite green.

@Zaldaryon

Copy link
Copy Markdown
Collaborator Author

All points from the review are addressed in b0f0aa2.

  1. Added relative positioning to the heading container in ModSuggestions.tsx:86, restoring contrast to 7.09:1 under the grid panel scrim.
  2. Stabilized the effect dependencies in useModSuggestions.ts:116 using installation and installed mods keys alongside refs. Mounting with consent and a 30 candidate catalog now executes a single pipeline run instead of firing twice upon syncModsCount.
  3. Detail resolution lookups on mount are capped at MAX_SUGGESTION_DETAIL_LOOKUPS (20) and pinned by DOM and unit tests.
  4. Reason selection in suggestions.ts:124 now scales each signal by its configured score weight, and tests verify that zeroing weights shifts the chosen explanation to the highest contributing term.
  5. Contrast test tests/text-contrast.test.ts now measures all foreground layers in ModSuggestions.tsx with foregrounds().
  6. Separated deduplication below the cap from the maximum size guard in tests/renderer/configReducer.test.ts, ensuring the includes check is directly tested.

Ready for re-review.

@Zaldaryon
Zaldaryon requested a review from Pixnop September 16, 2026 22:18
@Zaldaryon
Zaldaryon force-pushed the fix/issue-464-suggestions branch from b0f0aa2 to b2d09c9 Compare September 16, 2026 22:41
@Zaldaryon
Zaldaryon requested review from Pixnop and removed request for Pixnop September 16, 2026 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants