feat(gui): trust-first install readiness and resume/retry experience - #197
Open
oldrepublicwizard wants to merge 9 commits into
Open
feat(gui): trust-first install readiness and resume/retry experience#197oldrepublicwizard wants to merge 9 commits into
oldrepublicwizard wants to merge 9 commits into
Conversation
added 9 commits
July 29, 2026 21:10
Embedded <!--<<ModSync>> metadata blocks never carry a Name field (it lives in the markdown header), but DeserializeComponent required Name unconditionally, so parsing that block always threw and silently fell back to a lossy legacy parser that assigned a fresh random GUID. Thread an optional requireName parameter through DeserializeComponent, DeserializeYamlComponent, DeserializeModComponentFromTomlString, and ModComponent.DeserializeTomlComponent, defaulting to true everywhere except the two embedded-metadata-block parse paths in MarkdownParser. Also fix instruction/option grouping in ProcessInstructionsAndOptions and GroupKeyValuePairsIntoInstructions/GroupKeyValuePairsIntoOptions: boundary detection keyed on a hardcoded field name (Action/Name) broke when Guid legitimately preceded it in field order. Use duplicate-key detection instead, which is order-independent. Corrects MarkdownFileTests' own fixture, which was itself invalid YAML (brace-wrapped GUIDs parse as YAML flow-mappings; unindented fields de-indent out of their list item), and adds C2RoundtripInvariantTests as a self-contained regression guard for the invariant.
…params ProcessInstructionsAndOptions's inline Instructions/Options loops handled a KeyValuePair<string, object> case that can never execute there: that branch only runs when hasKeyValuePairs is false, and any KeyValuePair item's runtime type name always starts with "KeyValuePair", which would have made hasKeyValuePairs true. Removed the dead branch and the now- unused accumulator dictionaries. Also added XML doc comments for the requireName parameter on DeserializeYamlComponent, DeserializeModComponentFromTomlString, and ModComponent.DeserializeTomlComponent, which had gained it without documentation (DeserializeComponent already had one).
…API fixups) U1: remove the <Compile Remove> exclusions for DocumentationRoundTripTests.cs, MarkdownFileTests.cs, and MarkdownImportTests.cs in ModSync.Tests.csproj so they compile and run again. U2: fix the compile/runtime issues re-enabling them surfaced: - DocumentationRoundTripTests.cs: skip via Assert.Inconclusive when the test_modbuild_k1.md fixture is absent, instead of a hard Assert.Fail; fix a stale ModLinkFilenames reference to the current Dependencies property. - MarkdownImportTests.cs: skip via Assert.Inconclusive when the mod-builds corpus is absent; update to current ModComponent/Category APIs (no more CSharpKOTOR.ModComponent, Category is now a list); remove assertions on Instruction.Guid, which no longer exists on that type; drop assertions tied to corpus content that no longer matches the current fixture.
…be, --no-checkpoint wiring Adds ValidationPipelineOptions.InstallStartReadiness, an environment-only preset reused by the wizard's pre-install readiness summary instead of a second validation pipeline. Adds ResumableSessionInfo and CheckpointManager.ProbeResumableSessionAsync, a static probe against the on-disk install_session.json (destination path + overlapping component GUIDs) so the wizard can detect a resumable session on re-entry without needing in-memory session state. Wires up the previously-inert --no-checkpoint CLI flag through InstallCoordinator.InitializeAsync so it disables Git checkpoint commits while leaving session-based resume intact.
InstallStartPage now runs the InstallStartReadiness preset on every navigation, blocks Next while critical readiness errors remain, and probes for a resumable session at the destination on every re-entry -- offering Resume previous / Start over independent of in-memory state. InstallingPage surfaces which component is active, overall progress, and a healthy/failed/cancelled run state on every callback, and gates auto-restart behind an explicit Resume/Retry click instead of treating any exit as a completed run. Failure copy reports completed-vs-remaining counts and explicitly disclaims full pristine-folder restoration. Removes InstallationErrorDialog, whose three-way Rollback/Continue/Abort choice implied a full-folder restore this product doesn't deliver; its rollback-capable sibling (the Git checkpoint browser) remains separate, pre-existing tooling untouched by this change.
Adds headless coverage for the resume-offer branch that existed in production code but wasn't exercised: the offer blocking Next until a choice is made, Resume previous unblocking Next while preserving the on-disk session, and Start over clearing resumability and resetting component state.
Updates AGENTS.md and the validation/CLI knowledgebase docs to describe InstallStartPage's readiness check and resume offer, InstallingPage's honest success/failed/cancelled states, the InstallStartReadiness validation preset, and --no-checkpoint's actual (Git-commits-only) scope. Verified against the implementing code -- no drift found.
… reads Code review of the trust-first install experience surfaced three real bugs: - InstallingPage.ResumeRetryButton_Click reused the wizard dialog's single-lifetime CancellationTokenSource. Once "Stop Install" cancels it, that token stays cancelled for the dialog's remaining life, so a later Resume/Retry click passed an already-cancelled token into Task.Run -- which never invokes its delegate for an already-cancelled token, silently hanging the UI forever with no error. Resume/Retry is now hidden after an explicit Stop Install, directing the player to reopen the wizard instead, where InstallStartPage offers the same resume via the on-disk session file (unaffected by the dialog's spent token). - CheckpointManager.ProbeResumableSessionAsync deserialized install_session.json with no exception handling; a corrupted or truncated file (e.g. from a crash mid-write) threw an unhandled exception on the wizard's navigation path instead of failing closed. Now treated the same as "no session". - CheckpointManager.InitializeAsync adopted an existing session file without checking whether it was written for the current destination, unlike ProbeResumableSessionAsync which already made that check. A session file copied in from a different install (e.g. the whole game folder copied elsewhere) could silently donate its Completed/Skipped markers to an unrelated install, causing components to be skipped that were never actually installed at the new destination. Adds regression coverage for all three: a headless test asserting Resume/Retry is hidden after a cancellation-based failure, and two CheckpointManager tests for cross-destination session probing and adoption.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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
1 participant
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.
Summary
Long KOTOR builds used to fail with no clear signal beyond "start, fail, guess, restart." This adds a pre-install readiness summary that blocks advancing while environment blockers remain (bad path, missing HoloPatcher), always-visible install progress with an honest healthy/failed/cancelled state, and resume/retry that survives closing and reopening the wizard against the same destination -- without ever implying a one-click full-folder rollback.
InstallStartPageruns a fast, environment-only readiness check on every visit and probes the destination's on-diskinstall_session.jsonfor a resumable prior session, independent of in-memory state -- so reopening the wizard after a failure still offers Resume previous / Start over.InstallingPagereports which mod is active, overall progress, and a healthy/failed/cancelled run state; auto-restart is gated behind an explicit Resume/Retry click instead of treating any exit as a completed run. Failure copy reports completed-vs-remaining counts and never claims to restore a pristine game folder.--no-checkpointwas a documented CLI flag that did nothing; it now actually disables Git checkpoint commits while leaving session-based resume intact.InstallationErrorDialog, whose Rollback/Continue/Abort choice implied a full-folder restore this product doesn't deliver. The separate, pre-existing Git checkpoint browser (CheckpointManagementDialog) is untouched.Bugs found and fixed during review
A full multi-persona code review (correctness, testing, maintainability, reliability, adversarial, api-contract) surfaced three real defects, all fixed and covered by new tests:
Task.Run, which never runs its delegate -- the UI froze with no error. Resume/Retry is now hidden after an explicit Stop Install, directing the player to reopen the wizard instead.install_session.jsonwas deserialized with no exception handling; a truncated file (e.g. from a crash mid-write) threw on the wizard's navigation path. Now treated the same as "no session."CheckpointManager.InitializeAsyncadopted an existing session without checking it was written for the current destination (the resume-offer probe already did this check, but the actual install-start path didn't) -- copying a whole game folder elsewhere could cause components to be skipped as "already completed" that were never installed at the new location.Testing
InstallStartReadinessOptionsTests,NoCheckpointInstallTests,WizardResumeReentryTests(5, incl. 2 new destination-mismatch regression tests),InstallStartPageHeadlessTests(5, incl. 3 new Resume/Start-over interaction tests),InstallingPageHeadlessTests(4, incl. 1 new Stop-Install regression test),FomodGateHeadlessTests.Post-Deploy Monitoring & Validation