You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Four places in src/domain where the code does by hand what the codebase already has, or keeps a knob nobody turns: a semver comparator written out long-form while semver is a production dependency used three files over, two options bags no production caller fills, two exported functions for one table, and one predicate that restates the function below it.
Total estimated saving: about 100 lines of source, 4 exported concepts, one of three competing version orderings.
1. whatsNew.ts hand-rolls semver precedence that the installed semver package does
Sixty-five lines reimplementing semver precedence: dotted release parts, prerelease split, numeric identifiers sorting as numbers, numeric before alphanumeric, plain release outranking its own prerelease. semver is a production dependency and src/domain/versions/detect.ts, src/domain/mods/compatibility.ts and src/domain/mods/health.ts already import it.
Replace with:semver.compare behind a semver.valid guard, the exact shape src/renderer/src/utils/gameVersionOrder.ts already uses for the same problem. newestTagFirst becomes one line. Better still, move that renderer comparator into the domain so both call sites share it, which also ends the three-way split between compareVersions, compareGameVersionsDesc and compareWhatsNewVersions.
Savings: verified by applying it: the file goes from 402 to 351 lines and three private functions become one 8-line comparator.
Risk and test:semver.valid refuses a tag it cannot parse, where the hand-rolled parser coerced anything (Number(part) || 0). All 38 published tags are valid semver once the existing stripVersionPrefix runs, and the guard's fallback is the one compareGameVersionsDesc already ships. Pinned by tests/domain/appUpdate/whatsNew.test.ts:354 (beta.10 after beta.9, not equal to it) and :365 (a release outranking its own prerelease); both pass unchanged, as do all 67 tests under tests/domain/appUpdate/.
Note for the reviewer: PR #442 recorded a deliberate choice here ("No semver dependency ... A small comparator in whatsNew.ts does it by hand instead, the same way betaUpdates.ts reads a prerelease with a string check"). That is recent and deliberate, so it is the maintainer's call. The stated analogy is thin though: betaUpdates.ts's check is a genuine one-liner (.includes("-")), not a 65-line precedence implementation, and the decision does not engage with semver already being proven in this same layer for this same problem.
Worth: medium.
2. whatsNew.ts carries two options bags no production caller fills
The only production caller, src/renderer/src/features/info/hooks/useWhatsNew.ts (lines 55, 99, 142), calls all three with no options at all. Only the test file ever fills them.
Replace with: read the module constants directly (DEFAULT_WHATS_NEW_LIMITS, DEFAULT_MAX_RELEASES_TO_SHOW) and drop both interfaces.
Savings: 2 exported interfaces, 3 parameters, about 12 lines.
Risk and test:tests/domain/appUpdate/whatsNew.test.ts uses the limits at lines 231, 242, 246, 254, 267 and maxReleases at 382 and 411 to reach the truncation and surrogate-pair branches cheaply. Walking those seven sites: the two maxReleases cases test the override itself and simply go away with it, the maxBlocks cases need a number swap at most, and only the three maxBlockLength cases need larger literals (a body over 2000 characters, a bigger repeat count) to reach the same branches. Those branches must stay covered, and that is the real cost of this one.
Worth: medium.
3. expectedGameExecutables is gameExecutableCandidates mapped to its file names
src/domain/versions/gameExecutable.ts:35 and :72
Two exported functions for one table: expectedGameExecutables(os) returns the file-name list, gameExecutableCandidates(os) returns exactly that list paired with a launch mode. The only caller outside the pair is src/domain/versions/install.ts:101, which needs the names for an exists check; detect.ts and launch.ts both use the candidates form.
Replace with: keep gameExecutableCandidates as the single table and have install.ts's gameLanded read .fileName off it.
Savings: verified by applying it: gameExecutable.ts 77 to 62 lines, its test file 64 to 35, including the "names the exact same files and order as expectedGameExecutables" case that exists purely because there are two functions.
Risk and test: macOS must keep returning an empty list so gameLanded short-circuits to true rather than declaring every macOS install failed. gameExecutableCandidates("darwin") already returns [] on its own (pinned at tests/domain/versions/gameExecutable.test.ts:52), so the short-circuit does not depend on the deleted function. Full suite passes after the cut. Note that #457 cites expectedGameExecutables by name and by line range as existing infrastructure for its planned feature, so whoever does this should leave a line in #457.
Worth: medium.
4. hasActiveInstalledModFilters restates the expression below it
src/domain/mods/installedFilters.ts:143 and :153
hasActiveInstalledModFilters returns the three-axis OR, countActiveInstalledModFilters right below returns the same three conditions summed. The definition of "a filter is set" is written twice, and both are consumed by the same single page, src/renderer/src/features/installations/pages/ManageMods.tsx.
Savings: about 2 lines, and one duplicated definition. Small, so take it while already in the file.
Risk and test: if a fourth axis is added the two must not disagree, which is what this fixes. Pinned by tests/domain/mods/installedFilters.test.ts, which exercises both across all three axes.
Not recommended: the same finding proposed un-exporting matchesInstalledModFilters:134. Skip that half. Its 12 direct test call sites would each have to route through filterInstalledMods (wrap the mod in a one-element array, check the length), which is equal or more code and trades a pure-predicate unit test for an indirect one.
The hexagonal split (pure src/domain, src/ipc and src/main as host, the renderer through window.api and feature adapters), the path policy, the IPC validation at the boundary, the mutation-tested guards and the accessibility work are deliberate and stay. tests/security-boundaries.test.ts, tests/log-provenance.test.ts, tests/text-contrast.test.ts and tests/i18n/i18n-parity.test.ts pin log provenance, no HTML sinks, contrast floors and locale parity: none of the changes above relax them.
Summary
Four places in
src/domainwhere the code does by hand what the codebase already has, or keeps a knob nobody turns: a semver comparator written out long-form whilesemveris a production dependency used three files over, two options bags no production caller fills, two exported functions for one table, and one predicate that restates the function below it.Total estimated saving: about 100 lines of source, 4 exported concepts, one of three competing version orderings.
1.
whatsNew.tshand-rolls semver precedence that the installedsemverpackage doessrc/domain/appUpdate/whatsNew.ts:274(parseWhatsNewVersion),:287(compareIdentifiers),:310(compareWhatsNewVersions)Sixty-five lines reimplementing semver precedence: dotted release parts, prerelease split, numeric identifiers sorting as numbers, numeric before alphanumeric, plain release outranking its own prerelease.
semveris a production dependency andsrc/domain/versions/detect.ts,src/domain/mods/compatibility.tsandsrc/domain/mods/health.tsalready import it.Replace with:
semver.comparebehind asemver.validguard, the exact shapesrc/renderer/src/utils/gameVersionOrder.tsalready uses for the same problem.newestTagFirstbecomes one line. Better still, move that renderer comparator into the domain so both call sites share it, which also ends the three-way split betweencompareVersions,compareGameVersionsDescandcompareWhatsNewVersions.Savings: verified by applying it: the file goes from 402 to 351 lines and three private functions become one 8-line comparator.
Risk and test:
semver.validrefuses a tag it cannot parse, where the hand-rolled parser coerced anything (Number(part) || 0). All 38 published tags are valid semver once the existingstripVersionPrefixruns, and the guard's fallback is the onecompareGameVersionsDescalready ships. Pinned bytests/domain/appUpdate/whatsNew.test.ts:354(beta.10 after beta.9, not equal to it) and:365(a release outranking its own prerelease); both pass unchanged, as do all 67 tests undertests/domain/appUpdate/.Note for the reviewer: PR #442 recorded a deliberate choice here ("No semver dependency ... A small comparator in whatsNew.ts does it by hand instead, the same way betaUpdates.ts reads a prerelease with a string check"). That is recent and deliberate, so it is the maintainer's call. The stated analogy is thin though:
betaUpdates.ts's check is a genuine one-liner (.includes("-")), not a 65-line precedence implementation, and the decision does not engage withsemveralready being proven in this same layer for this same problem.Worth: medium.
2.
whatsNew.tscarries two options bags no production caller fillssrc/domain/appUpdate/whatsNew.ts:25(WhatsNewLimits),:175(releaseNotesToBlocks(markdown, limits)),:335(SelectReleasesOptions),:367and:395(optionsparameters)The only production caller,
src/renderer/src/features/info/hooks/useWhatsNew.ts(lines 55, 99, 142), calls all three with no options at all. Only the test file ever fills them.Replace with: read the module constants directly (
DEFAULT_WHATS_NEW_LIMITS,DEFAULT_MAX_RELEASES_TO_SHOW) and drop both interfaces.Savings: 2 exported interfaces, 3 parameters, about 12 lines.
Risk and test:
tests/domain/appUpdate/whatsNew.test.tsuses the limits at lines 231, 242, 246, 254, 267 andmaxReleasesat 382 and 411 to reach the truncation and surrogate-pair branches cheaply. Walking those seven sites: the twomaxReleasescases test the override itself and simply go away with it, themaxBlockscases need a number swap at most, and only the threemaxBlockLengthcases need larger literals (a body over 2000 characters, a bigger repeat count) to reach the same branches. Those branches must stay covered, and that is the real cost of this one.Worth: medium.
3.
expectedGameExecutablesisgameExecutableCandidatesmapped to its file namessrc/domain/versions/gameExecutable.ts:35and:72Two exported functions for one table:
expectedGameExecutables(os)returns the file-name list,gameExecutableCandidates(os)returns exactly that list paired with a launch mode. The only caller outside the pair issrc/domain/versions/install.ts:101, which needs the names for anexistscheck;detect.tsandlaunch.tsboth use the candidates form.Replace with: keep
gameExecutableCandidatesas the single table and haveinstall.ts'sgameLandedread.fileNameoff it.Savings: verified by applying it:
gameExecutable.ts77 to 62 lines, its test file 64 to 35, including the "names the exact same files and order as expectedGameExecutables" case that exists purely because there are two functions.Risk and test: macOS must keep returning an empty list so
gameLandedshort-circuits to true rather than declaring every macOS install failed.gameExecutableCandidates("darwin")already returns[]on its own (pinned attests/domain/versions/gameExecutable.test.ts:52), so the short-circuit does not depend on the deleted function. Full suite passes after the cut. Note that #457 citesexpectedGameExecutablesby name and by line range as existing infrastructure for its planned feature, so whoever does this should leave a line in #457.Worth: medium.
4.
hasActiveInstalledModFiltersrestates the expression below itsrc/domain/mods/installedFilters.ts:143and:153hasActiveInstalledModFiltersreturns the three-axis OR,countActiveInstalledModFiltersright below returns the same three conditions summed. The definition of "a filter is set" is written twice, and both are consumed by the same single page,src/renderer/src/features/installations/pages/ManageMods.tsx.Replace with:
hasActiveInstalledModFilters = (f) => countActiveInstalledModFilters(f) > 0.Savings: about 2 lines, and one duplicated definition. Small, so take it while already in the file.
Risk and test: if a fourth axis is added the two must not disagree, which is what this fixes. Pinned by
tests/domain/mods/installedFilters.test.ts, which exercises both across all three axes.Not recommended: the same finding proposed un-exporting
matchesInstalledModFilters:134. Skip that half. Its 12 direct test call sites would each have to route throughfilterInstalledMods(wrap the mod in a one-element array, check the length), which is equal or more code and trades a pure-predicate unit test for an indirect one.Worth: low.
Suggested order
expectedGameExecutables), self-contained and already verified end to end, leave a note on Optimum as a VS Version: recognise it now, install it from the launcher next #457.hasActiveInstalledModFilters), two lines, do it in passing.whatsNew.tsand its test.whatsNew.tsinstead of two.Out of scope
The hexagonal split (pure
src/domain,src/ipcandsrc/mainas host, the renderer throughwindow.apiand feature adapters), the path policy, the IPC validation at the boundary, the mutation-tested guards and the accessibility work are deliberate and stay.tests/security-boundaries.test.ts,tests/log-provenance.test.ts,tests/text-contrast.test.tsandtests/i18n/i18n-parity.test.tspin log provenance, no HTML sinks, contrast floors and locale parity: none of the changes above relax them.