Repository navigation
feat: rebuild Industry UI - #4
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af43fe7014
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| cardEntries.Clear(); | ||
| cards.Controls.Clear(); | ||
|
|
||
| BackupFolders folders = BackupFolders.Read(); |
There was a problem hiding this comment.
When a user selects a destination other than Data.DataRootDir, the backup is successfully written there, but this picker still obtains every candidate from BackupFolders.Read(), which only scans Data.DataRootDir. The repository has no browse/import path in the restore wizard, so backups created through the new destination selector disappear from Home, History, and Restore and cannot be restored through the application.
Useful? React with 👍 / 👎.
| { | ||
| try | ||
| { | ||
| Directory.Delete(item.Folder.Path, true); |
There was a problem hiding this comment.
During an active restore the History rail remains available, and pausing at a module boundary makes it straightforward to invoke this action. If the source is an uncompressed recognized user backup older than 90 days, it is included in candidates and deleted here; resuming then makes subsequent modules read a missing source after earlier modules have already changed the machine. Exclude the current restore source or disable pruning while RunCoordinator.IsRunning.
Useful? React with 👍 / 👎.
| if (!summaryShown) | ||
| { | ||
| ((IRunUi)this).ShowSummary( | ||
| RunSummary.For(new List<ModuleOutcome>(), false, verb, | ||
| "the " + caption.ToLowerInvariant() + " runner returned without a result"), | ||
| caption, | ||
| new List<ModuleOutcome>()); |
There was a problem hiding this comment.
When the user presses Cancel in the restore consent dialog, RunRestoreCore intentionally returns without calling ShowSummary; this fallback therefore renders the normal user cancellation as a problem stating that the runner returned without a result. Since the dedicated progress page is already visible by then, this path should show a canceled/no-changes result or return to the wizard rather than reporting an execution failure.
Useful? React with 👍 / 👎.
|
/oc review this pr |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 040367560c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| foreach (SourceFile source in sourceFiles) | ||
| File.Delete(source.FullPath); |
There was a problem hiding this comment.
When the default Fast compression is used and AppStoreApps succeeds, this loop deletes its root-level Remember installed apps.json after adding it to the payload. Unlike other modules, AppStoreApps.RestoreAsync ignores the extracted restore path and opens RestAppsForm, which enumerates Data.DataRootDir and reads AppStoreApps.ExportPathIn directly from the selected backup folder. Consequently, even a backup in the default destination appears in the restore wizard but the app-reinstall dialog cannot read its package list; pass the extracted source into that dialog or retain this artifact where its reader expects it.
Useful? React with 👍 / 👎.
| finally | ||
| { | ||
| handle.Free(); |
There was a problem hiding this comment.
PrivateFontCollection.AddMemoryFont requires the supplied memory to remain valid while the collection uses the font, but this frees the only pinned handle immediately and retains no reference to fontBytes. After EnsureLoaded returns, the GC may move or reclaim those arrays while controls throughout the application continue creating and drawing Barlow/IBM Plex fonts from Collection, leading to invalid font data or native drawing failures. Keep each pinned buffer alive until the private collection is disposed.
Useful? React with 👍 / 👎.
| BackupFolder latest = folders.Backups[0]; | ||
| IReadOnlyList<DriftItem> drifted = DetectDrift(latest, registrations, out string driftUnavailableReason); | ||
| string driftCount = drifted == null ? "Unavailable" : drifted.Count.ToString(); |
There was a problem hiding this comment.
With the new default Fast compression, this synchronous Home-page call reaches BackupPayload.TryPrepareForRead, which extracts every payload entry to a temporary directory before drift detection examines only the registry-capable modules. Since Home is rendered during startup and on every return to it, a backup containing large font or folder artifacts can make the UI appear hung and require temporary space equal to the full uncompressed snapshot merely to display the dashboard. Probe only the entries needed for drift, cache the prepared payload, or move extraction off the UI path.
Useful? React with 👍 / 👎.
| catch (Exception ex) | ||
| { | ||
| error = ex.Message; | ||
| return false; |
There was a problem hiding this comment.
If ZIP creation throws—for example because the destination fills up or a source file becomes unreadable—the catch returns without deleting the .payload-<guid>.tmp created above. These partial archives can be nearly as large as the backup, are explicitly excluded from later archive inputs, and InvalidateBackupManifest removes only payload.zip, so repeated attempts accumulate orphaned data in the backup folder and may exhaust the destination. Hoist the temporary path outside the try and remove it on every failure path.
Useful? React with 👍 / 👎.
|
I read the key changed files and built the solution. The build succeeds with Review1. Custom-destination backups are not discoverable for restore
Fix: either scan the manifest registry for all known destination paths, or remove custom destinations until the restore wizard can browse to them. 2. Pruning can delete the active restore source
Fix: guard the prune action with 3. Restore consent cancellation is reported as a runner failure
Fix: in 4. Minor:
|
…ration' into codex/winrestorekit-visual-exploration
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5d26758005
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return scopeGroups | ||
| .Where(group => scopeToggles[group].Checked) | ||
| .SelectMany(group => group.Modules) | ||
| .Distinct() |
There was a problem hiding this comment.
When the user selects the App settings group, this expansion runs every module in that group, including both EEnvironmentFiltered and the unrestricted EEnvironment. The latter explicitly exports environment variables such as API tokens and access keys in plaintext, but the rebuilt page provides neither a module-level opt-out nor the module's warning, so choosing the apparently generic group can expose credentials even when the user would have chosen the filtered variant in the previous module tree. Keep these alternatives independently selectable and surface the unrestricted export warning.
Useful? React with 👍 / 👎.
| foreach (string path in paths) | ||
| { | ||
| BackupFolder folder = new BackupFolder(path); | ||
|
|
||
| if (IsSnapshot(folder.Name)) | ||
| snapshots.Add(folder); | ||
| else | ||
| backups.Add(folder); |
There was a problem hiding this comment.
When a user chooses an existing general-purpose destination such as Documents or a drive root, every immediate child directory is converted into a BackupFolder, not just the timestamp folder created by WinRestoreKit. Because folders without manifests are intentionally accepted as legacy backups, unrelated Projects/Photos/etc. then appear in Home, History, and Restore; Home may recursively measure their entire contents, and Restore may offer them as unknown legacy sources. Record the exact created backup path or require recognizable backup artifacts when enumerating custom roots.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 08d777a0c4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| catch (Exception ex) | ||
| { | ||
| return new BackupFolders(backups, snapshots, ex.Message); |
There was a problem hiding this comment.
When a remembered USB drive, disconnected network share, or other custom root throws an IOException rather than DirectoryNotFoundException, this returns a non-null UnreadableReason even if the default root was already enumerated successfully. RestoreWizardStep1View.RefreshView treats any such reason as fatal and returns before adding the accumulated folders, so one offline custom destination makes every valid local backup disappear from Restore, Home, and History until that destination becomes available again. Handle failures per custom root while preserving readable roots, reserving the fatal result for the default root or for a state where nothing could be inspected.
Useful? React with 👍 / 👎.
| incompleteOutcomes); | ||
| return; | ||
| } | ||
| bool archived = BackupPayload.TryArchive(backupPath, compression, out string archiveError); |
There was a problem hiding this comment.
With the new default Fast compression, this synchronous call runs after RunModulesBackup resumes on the WinForms synchronization context, so compressing and verifying a large fonts/AppData snapshot blocks painting and input for the entire archive operation. The module loop has already reported 100% at this point, leaving the application apparently frozen at completion and making Pause/Cancel unusable during what may be a substantial part of the run; perform this work asynchronously and represent it as a progress phase.
Useful? React with 👍 / 👎.
| private void UpdateProgressButton(bool running) | ||
| { | ||
| btnProgress.Enabled = railEnabled && running; | ||
|
|
||
| private void btnHome_Click(object sender, EventArgs e) | ||
| => navigation.Show(homePage); | ||
| if (!btnProgress.Enabled) | ||
| btnProgress.IsSelected = false; |
There was a problem hiding this comment.
If the user navigates from the progress page to Home, Back up, Restore, or History while a run is active, those rail handlers call navigation.Show and clear the back stack. When the run then finishes, this disables the only rail control that can reopen progressPage, so the success/failure summary and per-module outcomes become inaccessible even though the page is retained in memory. Keep the progress entry enabled until the result is dismissed, or prevent navigation away during the run.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3c35a291b2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| detail = "Cancellation was requested. No further group was started. Completed output " + | ||
| "remains without a trusted manifest."; | ||
| } | ||
| else if (TryRemoveIncompleteBackupFolder(backupPath)) |
There was a problem hiding this comment.
When two app instances start a backup to the same destination during the same minute, both derive the same Data.NowShort path and can observe it as absent before either creates it. If the other instance creates or completes the folder first, this instance still has folderExistedBeforeRun == false, so canceling recursively deletes the shared folder here, including the other run's artifacts. Establish folder ownership atomically or use a unique path before removing canceled output.
Useful? React with 👍 / 👎.
| { | ||
| if (module.HasDriftedFrom(backupPath) == true) |
There was a problem hiding this comment.
When the latest backup contains only modules that cannot compare drift, or registry probing/export fails, every HasDriftedFrom call returns null; this loop silently drops those results and returns an empty non-null list. HomePageView consequently renders the run as measured with 0 drift and “No tracked changes found” rather than unavailable, even though no comparison succeeded. Propagate comparison coverage or an unknown result instead of collapsing it into zero drift.
Useful? React with 👍 / 👎.
| if (BackupPayload.TryPrepareForRead(restoreSourcePath, out payload, out string ignoredError)) | ||
| probePath = payload.Path; |
There was a problem hiding this comment.
For a compressed backup containing large Fonts or AppData artifacts, opening restore step 2 synchronously extracts every archive entry here on the WinForms thread, and the finally immediately deletes that extraction. Starting the restore then calls BackupPayload.TryPrepareForRead again and extracts the same archive a second time, blocking navigation and doubling temporary I/O and space churn before any module runs. Probe the manifest/archive entries directly or retain and reuse a prepared payload scope.
Useful? React with 👍 / 👎.
…ration' into codex/winrestorekit-visual-exploration
|
/oc check |
|
Based on the PR review findings, here's the assessment: Critical Issues (Must Fix)
High Priority
Medium Priority
RecommendationRequest changes for items 1-5. The custom-destination discoverability and prune-safety issues are design flaws that break core features; the font buffer issue can cause crashes on some systems. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a4c33b1b5e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return; | ||
| } | ||
|
|
||
| BackupFolder latest = folders.Backups[0]; |
There was a problem hiding this comment.
When a remembered shared/custom root contains a newer backup created by another machine or Windows account, this global newest entry becomes the dashboard baseline without checking its manifest provenance. The page then compares that foreign snapshot against the current registry and claims it was taken from the current machine, potentially telling users this PC is protected while reporting meaningless drift; select by manifest machine/user or clearly treat foreign backups as non-current.
Useful? React with 👍 / 👎.
| + watchedGroups.Count + " watched groups are under watch; " | ||
| + (drifted == null ? "drift could not be measured" : drifted.Count + " have drifted since the snapshot") + "."; | ||
|
|
||
| BuildHeader("SYSTEM IS CAPTURED", summary); |
There was a problem hiding this comment.
When the newest backup manifest records failed, skipped, or unknown modules—even every selected module failing—the dashboard still unconditionally renders SYSTEM IS CAPTURED. History already derives a Failed/Partial result from these same manifest states, so the Home status can falsely reassure users after an unsuccessful backup; gate this headline and protection summary on the latest manifest outcomes.
Useful? React with 👍 / 👎.
| if (destinationRoot != null) | ||
| BackupRootRegistry.Remember(destinationRoot); |
There was a problem hiding this comment.
For the first backup to a new custom destination, a crash or power loss during module execution or archiving occurs before this call, so the root is never persisted and the partial backup cannot be rediscovered after restart. The new BackupRootRegistry discovery is fresh evidence beyond the earlier complete-backup issue: completed roots are now found, but interrupted roots still disappear despite custom-folder recognition explicitly admitting interrupted timestamp folders; remember the root once its destination folder is created.
Useful? React with 👍 / 👎.
…ration' into codex/winrestorekit-visual-exploration
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff267b5e53
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!TryRemove(path) && File.Exists(path)) | ||
| { | ||
| logger.LogMessage( | ||
| "The previous backup manifest at " + path + " could not be removed or emptied. " | ||
| "The previous backup metadata or payload at " + path + " could not be removed or emptied. " | ||
| + "If this run does not finish, that file still describes the PREVIOUS run."); | ||
| } |
There was a problem hiding this comment.
When a second backup reuses the process-scoped timestamp folder while its previous payload.zip is locked against deletion and truncation, this path only logs the failure and continues. The new modules write loose artifacts, but TryArchive excludes the existing payload and cannot replace the locked file; the run then publishes a new uncompressed manifest, while TryPrepareForRead later sees the stale archive and restores the previous run's data instead. Abort the run or otherwise prevent a stale payload from surviving alongside the new manifest.
Useful? React with 👍 / 👎.
| if (!File.Exists(payloadPath)) | ||
| { | ||
| payload = new ReadScope(backupPath, null); | ||
| return true; |
There was a problem hiding this comment.
If a compressed backup's payload.zip was omitted during copying, deleted, or quarantined, this branch treats the metadata-only directory as a valid loose legacy backup even though its manifest declares that the module artifacts are archived. The wizard trusts the manifest's succeeded rows, so starting the restore can take snapshots and close applications before modules fail or do nothing against the empty root. Check the manifest's payload declaration and return a preparation error when the declared archive is missing.
Useful? React with 👍 / 👎.
| foreach (string path in paths) | ||
| { | ||
| if (!isDefaultRoot && !IsRecognizableCustomFolder(path)) | ||
| continue; | ||
|
|
||
| BackupFolder folder = new BackupFolder(path); |
There was a problem hiding this comment.
When the user chooses a destination beneath Data.DataRootDir, such as DataRootDir\Archive, the default-root pass admits Archive itself as a legacy backup because this filter is bypassed, and the remembered custom-root pass separately admits the timestamp folder inside it. The container consequently appears as a bogus restore source and Home/History count its contents again through the real child backup. Detect nested remembered roots or exclude their container directories during the default-root pass.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 71b7effb81
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| SnapshotCompression = compression; | ||
| string backupPath = Path.Combine(destinationPath, Data.NowShort); |
There was a problem hiding this comment.
When the chosen destination lies inside a folder being backed up—for example, selecting the VS Code snippets directory while capturing app settings—this makes the timestamped backup a descendant of that source. WindowsHelper.CopyFolderInto creates its destination before enumerating the source's subdirectories, so it then discovers the new backup directory and recursively copies the backup into itself until path-length or disk-space exhaustion. Validate the destination against the selected modules' source folders before starting the run.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05bbffc2a9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| void IRunUi.SetExplorerRestartVisible(bool visible) | ||
| { | ||
| if (visible) | ||
| UpdateUi(() => AppendLog("Explorer restart is required to apply restored settings.")); |
There was a problem hiding this comment.
When a restore successfully writes a module such as WTaskbar, this implementation only appends a log line and offers no way to call Utils.RestartExplorer(). The removed RunResultsPanel previously exposed that action, and WTaskbar depends on the post-import forced restart to prevent Explorer's in-memory pin list from overwriting the restored Taskband data on sign-out. Add the restart control and preserve its retry/result handling rather than merely logging the requirement.
Useful? React with 👍 / 👎.
|
|
||
| results.Add(outcome); | ||
|
|
||
| bool hasByteMeasurement = TryMeasureBackupArtifactBytes(folder, out long bytesWritten); |
There was a problem hiding this comment.
When a selected module writes a large folder tree, this call recursively re-enumerates every accumulated backup file after every module, and the await module.BackupAsync(...) continuation resumes on the WinForms synchronization context. A large Fonts/AppData snapshot therefore causes repeated O(modules × accumulated files) UI-thread stalls between groups, during which Pause and Cancel cannot respond. Measure incrementally or perform the recursive scan off the UI thread.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8d54055f15
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (BackupPayload.TryPrepareForRead(restoreSourcePath, out payload, out string ignoredError)) | ||
| probePath = payload.Path; |
There was a problem hiding this comment.
When an uncompressed backup is followed by a Fast/Max backup with a different selection in the same process, both runs reuse the frozen Data.NowShort folder, so artifacts from modules omitted by the second run remain and are swept into its new archive. Extracting that archive here lets HoldsSomethingFor find those stale artifacts despite their absence from the new manifest, causing the wizard to enable and preselect them and a restore to apply data from the earlier run. Either remove artifacts not produced by the current run before archiving or treat a valid manifest's omission as authoritative.
Useful? React with 👍 / 👎.
1 participant
Summary
Verification
dotnet build src/WinRestoreKit.sln --no-restoredotnet test src/WinRestoreKit.sln --no-restoreNotes
.gitignore, existing changelog exploration notes,.github/workflows/opencode.yml, and.superpowers/are not in this PR.