Skip to content

Add Sortium - #231

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

Add Sortium#231
SalvadorCorreia wants to merge 4 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.

@SalvadorCorreia

Copy link
Copy Markdown
Author

Over the last couple of days I have been working on addressing all issues raised by @Norphirion's comment. Today I was able to finish all the needed work.
Here is a summary of the updates:

1. Build Configuration
Pinned the TypeScript dependency in package.json and pnpm-workspace.yaml. Added the missing packages key to the workspace configuration, and stopped git-ignoring pnpm-lock.yaml to ensure deterministic builds in CI.

2. Queue Architecture (The 16-Minute Block)
The queue system has been completely rewritten.

  • Isolated Workers: It now spawns concurrent, independent processing loops per stream. A rate limit on Steam Hunters will no longer stall HowLongToBeat.
  • Negative Caching: Added a 3-strike retry limit. If an app repeatedly fails or returns a 404, it is negatively cached (marked as error and saved for 24 hours). This permanently resolves the infinite loop.
  • Visual Feedback: Added a visual indicator that lets the user know if a Stream is blocked. This prevents frustration from unknown blockage.

3. Plugin Unload / Dismount
Implemented the onDismount lifecycle hook. It properly unsubscribes from the history listener, iterates through a map of all injected React roots to trigger .unmount() and node removal, and flips a global state flag that safely breaks all background while loops.

4. Lua Backend Hardening

  • cache.lua now validates incoming stream_id requests against the internal registry table, closing the path traversal vulnerability.
  • hltb.lua and sh.lua now strictly enforce tonumber() on the incoming app_id before constructing the URL strings.

5. Transparency
Updated the README to explicitly mention that HowLongToBeat data is routed through the Augmented Steam API (api.augmentedsteam.com).

6. Minor Fixes

  • Deleted the dead hltb.ts file.
  • The non-functional library toggle has been disabled in the logic and hidden from the Settings UI.
  • Synced the versions in package.json and plugin.json to 1.0.0.
  • Removed the broken $schema link from the plugin manifest.

The build successfully passes from a clean checkout on my end.
Thank you again to @Norphirion for taking the time to perform this source audit.

@Norphirion

Copy link
Copy Markdown

Windows 11
Steam Client Beta build 1787097529 canal publicbeta
Millennium v3.5.0-beta.2
Node 24.18.0, pnpm 10.34.5

Re-checked on eac96ab, on Windows 11, Steam Client Beta build 1787097529, Millennium v3.5.0-beta.2, Node 24.18.0 and pnpm 10.34.5. I verified each item against the diff rather than taking the list at face value. All six hold, and on three of them you went further than I asked.

Build. typescript is pinned to 5.9.3 in devDependencies and again through overrides in pnpm-workspace.yaml, the missing packages key is there, and pnpm-lock.yaml is tracked and out of .gitignore. Reproduced the CI sequence: pnpm install --frozen-lockfile resolves TypeScript 5.9.3 and nothing else, and pnpm run build completes in about four seconds and exits cleanly. The ModuleKind crash is gone.

The queue. This is a real rewrite, not a patch. The setTimeout(r, 1000000) is gone entirely, queues and worker loops are keyed per stream so a rate limit on one no longer stalls the other, and handleTransientError counts three strikes before writing a negative cache entry that the 24 hour hard limit then respects.

The detail I want to call out, because it is the one that actually kills the infinite loop: a failing app is now pushed onto lowPriority, not back onto the high-priority stack. The old code re-pushed onto a LIFO and popped the same item on the very next iteration, so the retry could never make progress. Moving it to the FIFO tail is what makes the three-strike counter reachable at all. That is the right fix rather than the obvious one.

Unload. onDismount sets the flag, calls the stored historyUnsubscribe, then queueService.dismount() and cleanupInjectors(), which iterates a map of roots and calls unmount() plus node.remove() on each. The while (true) startup poll now breaks on the same flag, so nothing survives a disable.

Lua. is_valid_stream checks against the registry and is applied to load_stream, save_stream and clear_stream, so all three paths are closed rather than the two I named. In the stream modules, tonumber() gates the request and the URL is then built from tostring(numeric_id) rather than the original string, which is what actually normalises the value. Worth noting since a check alone would not have.

Transparency. The README now names api.augmentedsteam.com in the HowLongToBeat feature line.

Minor. hltb.ts is gone, the library toggle is commented out along with its handler so it no longer appears in settings, and both versions read 1.0.0. You also fixed something I did not raise: plugin.json previously declared useBackend: false alongside backendType: "lua", which contradicted itself for a plugin that does have a Lua backend. That is now consistent.

Two small things, neither blocking:

  • $schema was removed rather than corrected. The file moved rather than disappeared: https://raw.githubusercontent.com/SteamClientHomebrew/Millennium/main/src/system/plugin-schema.json returns 200. I had wrongly concluded it was gone on my own PR, so this is me correcting myself as much as suggesting anything. Removing it is perfectly fine if you prefer.
  • tonumber() also accepts "1.5" and "0x10", which would produce a valid but nonsensical URL. Not a security issue any more, since the value is normalised on the way out; an integer check would just be tighter.

Installed and used this time rather than just read. The six fixes all hold in practice, and the plugin does what it says. Three problems showed up in use, and two of them share a cause.

1. The Sortium grid paints over the collection filter editor

With the filter editor open, turning Sortium on visually swallows it: the editor's controls still show but its background is gone, and reopening the editor makes the controls flicker away and back without restoring it.

It is not a rendering glitch, it is an overflow. Measured on a live client with both open:

.sortium-grid (host)   top 133  bottom 133  height 0px    overflow: visible
  grid inside it       top 133  bottom 1971 height 1838px
CollectionEditor       top 164  bottom 573  height 409px

injectSortiumGrid sets sortiumGridDiv.style.height = '0px' on the host but leaves overflow at visible, so the 1838px grid escapes its zero-height box and paints over every sibling below it. The editor sits entirely inside that span.

It is worse than cosmetic: document.elementFromPoint() at the centre of the editor returns an <img> belonging to the Sortium grid, not the editor. So the filter editor is not just hidden there, it is unclickable while the Sortium view is on.

The reason it looks fine normally is that the toggle also hides Steam's own grid, so there is nothing left underneath for the overflow to cover. It works by accident rather than by construction.

Adding overflow: hidden to the host, or driving it with display: none instead of height: 0, should be enough.

2 and 3. The order churns for tens of seconds after sorting, and again when switching stream

These are the same bug seen twice. The grid subscribes with:

const unsubscribe = queueService.subscribe(() => {
    setRenderTrigger((prev) => prev + 1);
});

and notify() fires after every successful fetch in the worker loop, which paces itself at 500ms. So the whole list is re-sorted and re-rendered about twice a second for as long as the queue is draining. On a large library that is several minutes.

What makes it visible rather than subtle is where unknown values land:

if (value === null || value === undefined) {
    return direction === 'asc' ? Infinity : -Infinity;
}

Every app without data yet is parked at one end of the list, then jumps to its real position the moment its value arrives. One jump per fetched app, twice a second.

Switching metric from HLTB to Steam Hunters starts the same process over, because that stream's cache is cold, which is your third symptom rather than a separate one.

Worth saying: the underlying behaviour is correct, the data does converge and the final order is right. It is purely that sorting live while the data streams in makes the list unusable during the fill. A few directions, cheapest first: debounce the subscription so re-sorts happen at most every second or two; hold the previous position of apps that have no value yet instead of sending them to the end; or only re-sort once the active stream's queue has drained, with the incremental progress shown by the counter rather than by the list moving.

Also worth checking whether the grid needs to re-sort at all when the fetch that triggered the notify belongs to a stream other than the one currently displayed.

For completeness, since I have a plugin that injects into that same filter editor: I checked, and the element covering the editor is Sortium's, not mine.

@SalvadorCorreia

Copy link
Copy Markdown
Author

Thank you for verifying the initial fixes and for the continued testing. I am also grateful that we were able to discuss some of the fixes through discord. This comment will serve more as documentation.

Here is how I addressed the new findings:

1. The Sortium Grid & Collection Filter Overlap
Since I hadn't used Dynamic Collections, I missed the presence of the filter panel. Instead of applying overflow: hidden or display: none (which would break the DOM layout measurements my component relies on to mimic Steam's native CSS Grid), I fixed the root cause: the injection anchor.
SortiumGrid now specifically waits for and injects itself directly before GridWithControls, cleanly bypassing the filter editor entirely.

2 & 3. Sorting Churn & Visual Feedback
The live-sorting behavior is intentional. I prefer seeing the games jump into place as visual proof that the background fetch is working, rather than staring at a static list. However, I completely agree that without a dedicated UI indicator, it looks like a glitch.
To reach a middle ground, I added a new SortiumStreamStatus component next to the sorting toggles. While the active stream's queue is processing, it now displays an animated loading spinner alongside a (Resolved/Total) counter. It cleanly unmounts once the queue finishes draining.

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