Skip to content

Add Sortium - #231

Open
SalvadorCorreia wants to merge 2 commits into
SteamClientHomebrew:mainfrom
SalvadorCorreia:main
Open

Add Sortium#231
SalvadorCorreia wants to merge 2 commits into
SteamClientHomebrew:mainfrom
SalvadorCorreia:main

Conversation

@SalvadorCorreia

Copy link
Copy Markdown

Adds Sortium https://github.com/SalvadorCorreia/Sortium as a submodule under plugins/.

Sortium introduces advanced collection sorting to the Steam client using external data metrics.

Key Features

  • HowLongToBeat: Sorts games by Main Story, Main + Extras, Completionist, or All Styles.
  • Steam Hunters: Sorts games by Median Time, Fastest Time, Hunter Points, SteamDB Rating, or Achievement count.
  • Settings: Includes UI configuration, data stream toggles, and background data fetching to handle API rate limits.

MIT licensed.

Copilot AI lite review requested due to automatic review settings August 18, 2026 12:03
@github-actions github-actions Bot changed the title Add Sortium plugin Add Sortium Aug 18, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Adds the Sortium plugin to the Millennium Plugin Database as a Git submodule under plugins/, enabling advanced Steam library sorting using external completion/achievement metrics.

Changes:

  • Register plugins/sortium as a new submodule pointing to https://github.com/SalvadorCorreia/Sortium.
  • Configure the submodule to track the prod branch (via .gitmodules).

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread .gitmodules Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@Norphirion

Copy link
Copy Markdown

Disclosure: I am the author of #218 & #219 and tested this PR as part of the Community Contribution requirement.

Reviewed and built on Windows 11, Steam Client Beta, Millennium v3.5.0-beta.2, using the exact pinned plugin commit 0d3764e97cf0306af7f055efe010467eaae2866c. This is a source audit plus a build reproduction; I did not get as far as running the plugin inside Steam, because the pinned commit does not build (see the first point below).

Nice plugin, and an unusual one to review: a Lua backend that reaches out to third party APIs deserves a closer look than a display-only plugin, so I read all of it rather than skimming. Short version: I found nothing concerning on the security side, one blocker, and one bug I think you will want to fix before anyone runs a full sync.

1. Blocker: the build fails from a clean checkout

Reproduced exactly as the store CI runs it (npm install -g pnpm, then pnpm install, then pnpm run build), on Node 24 with pnpm 10.34.5:

TypeError: Cannot read properties of undefined (reading 'ES2015')
    at @rollup/plugin-typescript/dist/es/index.js:528:16
        ModuleKind.ES2015,

Cause. typescript is not declared anywhere in package.json, so it is only present transitively. Three copies end up in the tree, and pnpm why typescript shows @rollup/plugin-typescript@12.3.0 being paired with typescript@7.0.2:

typescript@4.9.5   <- @rollup/plugin-typescript@11.1.6
typescript@5.9.3   <- @rollup/plugin-typescript@12.3.0 peer (variation 1)
typescript@7.0.2   <- @rollup/plugin-typescript@12.3.0 peer (variation 2)

TypeScript 7 no longer exposes ModuleKind the way the plugin expects, hence the crash. Nothing in the repository pins the resolution, so the build depends on whatever the registry serves that day.

Fix, verified. Declaring TypeScript explicitly is enough. With only this change, pnpm install && pnpm run build succeeds and produces .millennium/:

"devDependencies": {
    "typescript": "5.9.3"
},
"pnpm": {
    "overrides": {
        "typescript": "5.9.3"
    }
}

I hit precisely this on my own plugin, and a reviewer caught it the same way, so this is me passing the favour along rather than anything clever.

Related. pnpm-lock.yaml is listed in .gitignore, and every dependency uses a caret range. Committing the lockfile would make the CI build reproducible rather than dependent on resolution date. Your tsconfig.json already sets moduleResolution: bundler, which is the other half of surviving TypeScript 7, so you are most of the way there.

Minor. pnpm-workspace.yaml has no packages: key, which pnpm 9 rejects outright with ERROR packages field missing or empty. CI installs the latest pnpm so it is unaffected, but contributors on pnpm 9 cannot install at all.

2. Serious: one failed request parks the whole queue

frontend/services/queue.ts, lines 180 and 195:

if (isHigh) this.highPriority.push(target);
else this.lowPriority.push(target);
await new Promise((r) => setTimeout(r, 1000000));

1000000 milliseconds is 16 minutes 40 seconds. It reads like a typo for 1000.

It runs on any failure not classified as a rate limit, so anything outside 429, 500, timeout and internal server error: a 403 challenge, a DNS failure, an offline client, a corporate proxy, a malformed body.

The queue is a single sequential loop, so this does not delay one item, it stops everything behind it for nearly 17 minutes.

The failing target is also pushed back before the sleep, and the high priority queue is LIFO (pop). The next iteration therefore takes the same item again. A persistent error parks the queue indefinitely, retrying one app every 16 minutes and never reaching the rest. Uncached items are all high priority, which is exactly the state a first Force Sync starts in.

For what it is worth, I could not trigger it through unknown app ids: both api.augmentedsteam.com and steamhunters.com answer 200 even for 999999999, so the missing-game path is handled gracefully. The realistic triggers are network unavailability and 403.

Suggested shape: a short backoff, a retry cap per app, and moving a repeatedly failing app out of the queue instead of back onto the top of it.

3. No cleanup on unload

definePlugin returns { title, icon, content } with no onDismount. After a disable or a reload, these keep running:

  • the navigation listener from MainWindowBrowserManager.m_history.listen(...), whose returned unsubscribe function is discarded
  • the React roots created by injectCollectionToggle and injectSortiumGrid, and the DOM nodes they are mounted on
  • startProcessing() and startRecoveryLoop(), both unbounded while loops
  • the while (true) startup poll in OnPopupCreation, which has no timeout

Plugin supports onDismount. Returning one that unsubscribes, unmounts the roots, removes the injected nodes and sets a stop flag on the loops would make a disable actually disable. This one was also raised on my own plugin, so I am not throwing stones.

4. Hardening notes on the Lua backend

None of these are remotely exploitable. They matter because the backend is not sandboxed the way the frontend is.

Path built from unvalidated IPC input. backend/cache.lua:

local function get_cache_path(stream_id)
    return millennium.get_install_path() .. "/cache_" .. stream_id .. ".json"
end

stream_id arrives from the frontend through GetCacheBatch and AppendToCache and is concatenated straight into a path. A caller passing ../../.. reads or writes JSON outside the plugin directory. Reaching it means already running JavaScript in Steam's context, so this is defence in depth rather than a hole, but validating stream_id against streams.registry costs two lines, and FetchStreamData already does exactly that lookup.

App id interpolated into the URL. backend/streams/hltb.lua and sh.lua build .. tostring(app_id) .. with no check that it is numeric. Impact is limited to reaching other paths on those two hosts, but tonumber() would close it.

Correct already: no metadata.json committed, .millennium/ ignored, MIT licence present and matching package.json, and no install scripts in package.json.

5. Transparency: the HLTB data comes from a third party

The feature is presented as HowLongToBeat and the metric ids are hltb_*, but the request goes to api.augmentedsteam.com, which is the Augmented Steam project rather than HowLongToBeat itself. Worth naming in the README and the store description, since it is a third party receiving the app ids of whatever a user sorts, and since the feature breaks if that API changes shape.

To state the thing a reviewer should actually answer: only the app id leaves the machine, over HTTPS, to two hardcoded hosts. No SteamID, no account identifier, no library listing, no telemetry, no analytics. I grepped for the usual suspects and found none: no eval, no new Function, no innerHTML, no dynamic import(), no WebSocket, and on the Lua side no os.execute, no io.popen, no loadstring.

6. Smaller points

  • frontend/services/hltb.ts appears to be dead code. Nothing imports it, and it holds a second fetch to the same API that bypasses the queue and the cache.
  • enableLibraryButton is exposed as a setting, but the branch using it is disabled with && false and injectHomeDropdowns is commented out. A toggle that does nothing is worth hiding until the feature returns.
  • plugin.json and package.json say 0.1.0, while the PR and the pinned commit message talk about v1.0.0.
  • $schema in plugin.json points at Millennium/main/src/sys/plugin-schema.json, which no longer exists upstream. Inherited from the official template and affects most plugins, so not really yours to fix, but it does 404.

Happy to re-test once the build is sorted, and to take screenshots of the sort views for the PR if that helps.

@SalvadorCorreia

Copy link
Copy Markdown
Author

@Norphirion, thank you very much for your thorough analysis of my codebase and for your detailed feedback
I have read your comment and already have planned solutions for most points. I will try to update my code as fast as possible and would love your support in rechecking it afterward!
I hope you have a good rest of your day.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants