One launch path, one filter object, one task runner - #501
Merged
Merged
Conversation
startInstall was copied from startExtract and kept its locale keys: success said "Unpacked", failure blamed a damaged archive with no room on the drive. It runs a Windows installer, not an archive, so neither sentence matches the InstallerRunResult reasons the catch classifies (timed out, missing, wrong platform). Add notifications.body.installed and notifications.body.installError to en-US and all 14 locale files, worded for an installer run. The failure sentence points at the log rather than the archive, since `reason` already carries the specific cause to the Activity Center row. i18n-parity.test.ts enforces en-US and fr-FR; the other 12 are kept for consistency with every sibling notifications.body key. taskManagerStartInstall.test.tsx gets one new assertion on the resolved failure body, since no test today pinned the wording.
MainMenu's PlayHandler and useLaunchGame drifted apart after PR #475: the hook kept the extraction, MainMenu's copy grew a "see report" notification action off outcomeNotification.report that navigates to the installation's session report, and the hook's copy of the same notification only ever read .link. Joining a server through useLaunchGame therefore offered no report link after a crash, unlike Play. Add useNavigate and the same report-action construction to useLaunchGame's outcome handling, so both launch surfaces build the same actions array. MainMenu still owns its own PlayHandler at this point, so this commit only changes behaviour on the Join path. manageInstallationServers.test.tsx gets a new test mirroring launchPlayGame.test.tsx's "offers the session report" case: a WhereProbe sibling of ManageInstallationServers reads useLocation, and clicking "See what went wrong" after Join is asserted to navigate to the installation's report page.
PR #475 lifted PlayHandler out of MainMenu into useLaunchGame so Play and Join would not carry two copies of the launch, the prevent-close token, the auto-backup question and the playtime stamp among them, but a later commit in the same PR backed the MainMenu side out again. The previous commit ported the one behaviour the two copies had drifted on (the session-report action), so both copies were behaviourally equal; this commit removes the one that no longer needs to exist. MainMenu now calls useLaunchGame() for launchGame and the backup prompt state, and renders the shared LaunchBackupPrompt instead of its own inline PopupDialogPanel copy. Its own PlayHandler, the skipBackupPromptOpen/skipBackupAnswerRef state and teardown effect are gone with it, along with the imports only they used (309 lines down to 136). launchPlayGame.test.tsx (717 lines) passes unchanged through the new call path, and its pass/fail set is identical to the previous commit's run: that is the proof nothing observable moved.
Nine useState calls, nine setter wrappers that all called the same
updateFilter helper, and nineteen props to ModsFilterBar (nine
value/setter pairs plus onClearFilters, declared once in the
signature and once in its Readonly type). The nine fields already
lived together in ModsBrowseState, which modsBrowseState.ts now
exposes as its own ModsFilters type.
ListMods keeps one useState<ModsFilters>, seeded from the same
getModsBrowseState() snapshot as before, and one setFilter(key,
value) replacing the nine setXFilter wrappers. It preserves
updateFilter's resolve-in-the-event contract (a state updater runs
during render, twice under StrictMode, and must not scroll or write
the snapshot there), resolved against a filtersRef mirror rather than
the render-scoped filters value: OrderFilter's changeOrder calls
setOrderBy and setOrderByOrder synchronously in one click, and
resolving the second call against the first call's own result (not
the stale value the render started with) is what keeps both fields
from a plain-object setState clobbering one another in the same
event.
ModsFilterBar takes {filters, setFilter, onClearFilters} and adapts
each child filter's own value/setter props internally. The debounced
query effect's nine-primitive dependency array collapses to one
filters object, correct because setFilter only produces a new
reference on a real change.
modsListModsFilterUpdate.test.tsx (including the StrictMode
once-per-change case) and modsBrowseActions.test.tsx pass unchanged,
and the full renderer-dom suite (82 files, 806 tests) is green.
The four task runners were the same scaffold with one line changed:
uuid, prevent-close add/remove, two info logs, ADD_TASK, await the
host call, a COMPLETED dispatch and optional toast on success,
classifyFailure and a failed dispatch and optional toast on catch.
166 lines of bodies, most of it repeated four times.
Add runTask({type, name, desc, notifications, messageKeys},
operation, onFinish) holding that scaffold. Each of the four becomes
a thin wrapper passing its own operation closure (the host call plus
whatever only it needs after: extract's post-await chmod, install's
throw-on-!result.ok) and its own two-key message pair. onFinish stays
two-argument (status, error) inside runTask; download's own
three-argument contract (status, path, error) is adapted in its own
wrapper closure, over the downloaded file's path, which only that
closure holds. compress's optional compressionLevel is still just an
extra argument on its own host call. The raw caught value reaches
each wrapper unchanged, so a wrapper's own `${err}` interpolation into
its error message is byte-for-byte what it produced before this fold.
TaskContextType's four signatures are untouched, so the four feature
adapters that call them positionally see no change.
Two of the runner's own info logs are now worded a little more
generically than their four separate copies were: a shared "runTask"
tag instead of "startDownload"/"startExtract"/etc., and compress's
"Adding" log no longer names its source path, only its destination.
Neither reaches a player, no test asserts either one's exact text, and
log-provenance.test.ts (which does check every logMessage call for a
leaked path or name) stays green. Every other log line, and both
prevent-close reason strings, resolve to the exact same text as
before through a small noun table for the three irregular ones
(extraction, installation, compression).
taskManagerFlows.test.tsx and taskManagerStartInstall.test.tsx pass
unchanged (41 tests, identical pass set before and after), pinning
the dispatch sequence, the toast type and the per-task error message
text for all four. The full renderer-dom suite (83 files, 807 tests)
and log-provenance.test.ts are green.
useConfigFolderPicker and useVersionInstallFolder's browseFolder each wrote out the same selectFolderDialog/checkPathEmpty/warn sequence usePickEmptyFolder already implements, byte-identical across all three. useInstallationFolder already shares it; these two now do too. useConfigFolderPicker becomes the dispatch line only, still dispatched whether or not the pick warned: usePickEmptyFolder does not block on a non-empty folder, and the config picker must not turn that into a block of its own. useVersionInstallFolder's browseFolder becomes pick, then setFolder/setFolderByUser. No test exercised any of the three pickers' warning notification itself before this fold, only that a pick was not blocked by it (installationsAddFolderPick.test.tsx's existing "warns without blocking" case checks the field still takes the path, never that the notification appears). Add that missing assertion for the hook every caller now shares. installationsAddFolderPick.test.tsx (46 tests plus the one new one), installationsAddFolderFollowsName.test.tsx, versionsAddVersion.test.tsx and configPageBackground.test.tsx pass unchanged, and the full renderer-dom suite (82 files, 807 tests) is green.
Measured against the 2026-09-15 dev run (id 34959134901, all green): build's npm run build:unpack step is npm run build (typecheck && electron-vite build) plus electron-builder --dir, so every build leg re-ran the full typecheck the dedicated typecheck job had already run on its own: 24s on typecheck's own leg, embedded again inside a 53s (ubuntu) / 48s (windows) build step. Separately, sonarcloud ran npm run test:coverage from scratch (3m57s) to reproduce the same coverage test-matrix (ubuntu-latest) had already produced (3m2s) a few minutes earlier in the same run. build now calls electron-vite build and electron-builder --dir directly instead of npm run build:unpack, skipping the embedded typecheck; npm run build:unpack itself is untouched for anyone running it outside CI. test-matrix's ubuntu-latest leg uploads its coverage/lcov.info as an artifact, and sonarcloud downloads it instead of regenerating it, needing that job so there is always something to download before it tries. continue-on-error keeps a failed or skipped test-matrix from turning sonarcloud into a hard CI failure; it already never blocked a merge. Both `&&` chains propagate a real build failure's exit code (verified locally: electron-vite build && electron-builder --dir stops and exits non-zero the moment the first command does), and a local run of the new build step against this branch produces the same dist/linux-unpacked output build:unpack always did. Estimated saving: sonarcloud loses its longest step (about 4 minutes), each build leg loses 20-25s of duplicate typechecking (worth roughly double that on the 2x-billed windows leg), for about 4.5 to 5 minutes off a run that currently spends close to 23 minutes of total job time. build-macos keeps npm run build:unpack: it is workflow_dispatch-only and not part of the measured run, so left alone for its own pass.
sonarcloud gained `needs: [test-matrix]` so it could read that job's coverage artifact instead of regenerating it, but its `if:` named none of success()/always()/failure()/cancelled(). GitHub Actions applies an implicit success() to a plain `if:` combined with `needs` (the `test` job's own comment above already documents this rule), so a single red or cancelled test-matrix leg, a Windows-only flake included, made sonarcloud SKIP outright instead of attempting the download and failing gracefully via the existing continue-on-error. On dev, sonarcloud had no `needs` and always attempted its own independent coverage run regardless of anything else in the workflow; always() restores that always-attempts behaviour on top of the artifact reuse.
Pixnop
force-pushed
the
chore/490-one-launch-path
branch
from
September 15, 2026 20:57
31278c2 to
fb299c6
Compare
Pixnop
marked this pull request as draft
September 15, 2026 20:59
Pixnop
marked this pull request as ready for review
September 15, 2026 21:00
3 tasks
Zaldaryon
approved these changes
Sep 15, 2026
Zaldaryon
left a comment
Zaldaryon
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed the current head. The launch and task paths now share the intended orchestration while preserving the existing callbacks, notifications, close guards, and launch state cleanup. The required checks pass, including typecheck, lint, the Ubuntu and Windows test matrix, build, SonarCloud, and the gate test. Focused tests pass with 37 tests. Approving.
Translation statusen-US is the source and carries 900 keys.
The status page is out of date. Refresh it with Drafted values are the machine-drafted ones still waiting for a native review, listed per locale in |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes
en-US.json+ all 14 locales,TaskManagerContext.tsx): two new keys,notifications.body.installed/installError, replacingstartInstall's reuse of the extract wording. +37/-2 lines, no restructuring.MainMenu.tsx,useLaunchGame.ts): the session-report notification action moved intouseLaunchGame(commit 1), thenMainMenu's ownPlayHandlerand its inline backup-prompt copy were deleted in favour of the shared hook (commit 2).MainMenu.tsx309 -> 136 lines.ListMods.tsx,ModsFilterBar.tsx,modsBrowseState.ts): nineuseStatecalls and nine setter wrappers collapsed into oneModsFiltersobject and onesetFilter.ListMods.tsx577 -> 552,ModsFilterBar.tsx101 -> 71.TaskManagerContext.tsx):startDownload/startExtract/startInstall/startCompressnow wrap one sharedrunTask, each keeping its own real difference (extract's chmod, install's throw-on-!result.ok, compress'scompressionLevel, download's three-argumentonFinish). 534 -> 519 lines, most of the drop coming out of the four bodies' repeated scaffolding.useConfigFolderPicker.ts,useVersionInstallFolder.ts): both now call the already-sharedusePickEmptyFolderinstead of repeating its pick/warn sequence..github/workflows/ci.yml):build's two legs callelectron-vite buildandelectron-builder --dirdirectly instead ofnpm run build:unpack, skipping the embeddednpm run typecheckthe dedicatedtypecheckjob already runs;sonarclouddownloadstest-matrix (ubuntu-latest)'s coverage artifact instead of regenerating it.Left out: item 6 (a mods-only
useExternalLinkswrapping the app-wide one). The issue's own "take it or leave it" framing on a 7-line, no-alias-collision hook held up on a second look, so it stays as-is rather than widening this PR for negligible savings.What does not change
Every fold's before/after test run is diffed, not just re-run:
launchPlayGame.test.tsx(717 lines, MainMenu's launch path) andtaskManagerFlows.test.tsx/taskManagerStartInstall.test.tsx(the four task runners' dispatch sequence, toast type and per-task error text) pass with an identical pass/fail set before and after their folds, which is the proof nothing observable moved on the paths those files pin.modsListModsFilterUpdate.test.tsx's StrictMode "once per filter change" case (the one that would catch a stale-snapshot or double-reset regression in the filter fold) is green. The fullrenderer-domsuite sits at 82 files / 807 tests through every renderer fold.A headless run of the packaged build at 1280x720 (seeded with a fake Installation and game version,
NEVER_LAUNCHED, noPlay/Joinclick made) covered the four surfaces the folds touch:useLaunchGame()call path: 490-home-play.pngManageInstallationServers: 490-manage-servers-join.pngaria-pressedflips, list empties with none favorited), reordered by Downloads (exercisesOrderFilter'schangeOrder, which setsorderByandorderByOrderin the same click, the casefiltersRefexists for), and Clear filters reset everything: 490-mods-browse-filters.pngrunTask-backedstartDownloadend to end (real network download,ADD_TASK, progress,COMPLETED) and landed the mod in the Installation's Mods folder (4 Mods -> 5), shown completed in the Activity Center: 490-task-runner-activity-center.pngFor the CI fold: read the changed workflow closely, validated it with
python3 -c "import yaml; yaml.safe_load(...)", and ran the new build step locally (electron-vite build && electron-builder --dir, both fromnode_modules/.bin). It produced the samedist/linux-unpackedoutputbuild:unpackalways did, and a deliberately broken first command in the chain confirmed the non-zero exit still propagates.actisn't installed, so the real check is this PR's own CI run.Bugs fixed on the way
startInstalltold players an installer failure was a damaged archive ("Check that the archive is not damaged..."); it now has its own wording that points at the log, sincereasonalready carries the specific cause to the Activity Center row.ManageInstallationServersoffered no "see what went wrong" link after a crash, unlike Play:MainMenu's copy had it,useLaunchGame's didn't, ever since PR Bookmark servers per Installation and join one from the launcher #475 backed theMainMenuside of the extraction out. Both surfaces now share the one launch path, and both offer the report link.Review
.github/workflows/ci.yml:94-95).sonarcloudpicked upneeds: [test-matrix]so it could read that job's coverage artifact instead of regenerating it, but itsif:named none ofsuccess()/always()/failure()/cancelled(). GitHub Actions applies an implicitsuccess()to a plainif:combined withneeds(thetestjob's own comment two jobs above already documents this exact rule), so a single red or cancelledtest-matrixleg, a Windows-only flake included, madesonarcloudSKIP outright instead of attempting the artifact download and failing gracefully through the existingcontinue-on-error. On dev,sonarcloudhad noneedsand always attempted its own independent coverage run regardless of the rest of the workflow, so this was a real, undisclosed change to "the CI still runs every check exactly once". Fixed by prefixing the condition withalways()(.github/workflows/ci.yml:95), restoring the always-attempts behaviour on top of the artifact reuse. New testtests/config/ci-sonarcloud-runs-on-failure.test.tsfails withoutalways()in theif:line and passes with it; this PR's own CI run (id 35013651666) is the live proof, sonarcloud running to completion rather than skipping.Testing
Gate:
npm run typecheckclean,npm run lint:ci0 errors / 14 pre-existing warnings,npm run format:checkclean,npm run test:coverage235 files / 4431 passed / 2 skipped (lines 96.2%, statements 94.6%, functions 95.4%, branches 91.2%, all above the configured floors).CI minutes (#491's fold, measured against dev run 34959134901, 2026-09-15, all green):
build'snpm run build:unpackstep embedded a fullnpm run typecheckinside itself, 24s on typecheck's own leg, re-run again inside a 53s (ubuntu) / 48s (windows) build step.sonarcloudrannpm run test:coveragefrom scratch (3m57s) to reproduce coveragetest-matrix (ubuntu-latest)had already produced (3m2s) minutes earlier in the same run. Estimated saving: sonarcloud loses its longest step (about 4 minutes), each build leg loses about 20-25s of duplicate typechecking (roughly double that on the 2x-billed windows leg), for about 4.5 to 5 minutes off a run that currently spends close to 23 minutes of total job time.This PR's own CI run confirms it: typecheck 30s, lint 51s, build (ubuntu-latest) 40s, build (windows-latest) 1m21s, test-matrix (ubuntu-latest) 3m22s, test-matrix (windows-latest) 4m18s, test 4s, sonarcloud 1m21s, all green, sonarcloud included rather than silently skipped.
Closes #490. Part of #492.