feat: WPF Timeline + Compare shell with Application layer extraction - #7
feat: WPF Timeline + Compare shell with Application layer extraction#7nicolasestrem wants to merge 4 commits into
Conversation
…sh fixes Add a new WPF application (WinRestoreKit.Wpf) implementing the approved Timeline + Comparison Workspace design, running side-by-side with the existing WinForms shell. Both share the same Core engine and a new WinRestoreKit.Application library. Architecture: - WinRestoreKit.Application: orchestration, backup presets, scope groups, snapshot event catalog, comparison service, update/theme services extracted from the WinForms project and shared by both shells - WinRestoreKit.Wpf: Timeline home, Compare/Confirm restore workflow, Backup workspace with presets, Progress with live metrics, Results, Advanced History, Settings, About, App Restore dialog - WinRestoreKit.Core: unchanged isolation guarantee (no UI dependency) - WinRestoreKit (WinForms): updated to reference Application layer Visual system: - Neutral Windows surfaces, mineral-blue actions, restrained coral warnings - Light/Dark/Follow-system themes with shared token set (~35 colours each) - Segoe UI Variable + IBM Plex Mono typography - Vector status glyphs (eliminates raw ErrorCircle enum text at source) - Full implicit control templates, visible keyboard focus, accessible names - 6-10px radii, flat panes, no gradients/glow/blur Fixed: - AboutView: Run.Text bound CurrentVersion read-only (TwoWay default) -> Mode=OneWay - ProgressWorkspaceView: ProgressBar.Value bound Percent read-only -> Mode=OneWay - Both crash with InvalidOperationException on view render; regression tests added Tests: 980 passed, 0 failed, 0 skipped (Release) Build: 0 warnings, 0 errors
|
@claude review |
|
/oc review |
|
Claude finished @nicolasestrem's task in 5m 9s —— View job Review complete
This is a large, well-executed PR — the extraction respects the codebase's core invariants (Application layer is genuinely UI-agnostic, 1. Test coverage regression on
Concrete failure scenario: a future change could flip either branch to 2. Minor asymmetry — WinForms owner cast (informational, not currently reachable) 3. Everything else checked out clean:
|
|
Here's my review of PR #7. Review: WPF Timeline + Compare Shell with Application Layer ExtractionArchitecture: StrongThe five-project solution (
IssuesMedium: Implicit
Medium: Lines 100-141 use Low:
Low: ViewModel base class inconsistency
Low:
Informational:
Informational: Not unit-testable through the ViewModel. Acceptable since WPF has no built-in folder dialog MVVM binding. What's Done Well
VerdictThis is a well-structured PR. The architecture is sound, the test coverage is strong, and the MVVM implementation is solid. The two medium issues (transitive Newtonsoft.Json dependency, untested scope group coverage) are worth addressing but not blockers. The low/informational items are code cleanliness concerns for a follow-up. Recommendation: Approve with the two medium suggestions noted as follow-up items. |
![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: 34a9bab938
ℹ️ 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".
| private void SystemEvents_UserPreferenceChanged(object sender, UserPreferenceChangedEventArgs e) | ||
| { | ||
| if (Mode == ThemeMode.FollowSystem) | ||
| ApplyEffectiveMode(); |
There was a problem hiding this comment.
When the default FollowSystem mode is active, SystemEvents.UserPreferenceChanged can invoke this handler from the SystemEvents thread. Calling ApplyEffectiveMode() there loads and replaces WPF resource dictionaries off the UI dispatcher, which can raise a cross-thread exception and terminate the application when a Windows preference or theme changes; dispatch this work to the application dispatcher as the existing WinForms handler does.
Useful? React with 👍 / 👎.
| if (isDefaultRoot) | ||
| return new BackupFolders(backups, snapshots, ex.Message); | ||
| { | ||
| return new BackupFolders(backups, snapshots, ex.Message, unreadableRoots); |
There was a problem hiding this comment.
If the executable-adjacent default backup root exists but cannot be enumerated—for example because of its ACL—this return exits the entire multi-root scan before any remembered custom destination is inspected. Valid snapshots on external or user-selected roots consequently disappear from Timeline and cannot be selected for restore; record the default-root error but continue through the remaining 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: d9cbcac3d7
ℹ️ 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".
| PreviewKeyDown="OnPreviewKeyDown" | ||
| SelectionChanged="OnSelectionChanged"> |
There was a problem hiding this comment.
A mouse click only changes SelectedEvent and focuses the list; OpenSelectedAsync is invoked exclusively from the Enter-key handler. Because the row template contains no button and the list has no double-click handler, mouse users cannot open either Compare or a failed-entry diagnostic from the Timeline. Bind a row activation gesture or explicit action to the same open operation.
Useful? React with 👍 / 👎.
| ShowSettingsCommand = new DelegateCommand(_ => NavigateTo(settings, "Settings")); | ||
| ShowAboutCommand = new DelegateCommand(_ => NavigateTo(about, "About")); |
There was a problem hiding this comment.
When a backup or restore is running, these commands remain enabled even though only CreateSnapshotCommand observes RunCoordinator.IsRunning. Clicking Settings or About replaces the progress/confirmation workspace, and the shell exposes no command for returning to the active operation, so the user loses access to pause, cancellation, logs, and status while a potentially destructive run continues. Disable these navigation commands during a run or provide an active-run navigation route.
Useful? React with 👍 / 👎.
| { | ||
| CreateSnapshotCommand = new DelegateCommand(_ => ShowCreateSnapshot(), | ||
| _ => !RunCoordinator.IsRunning); | ||
| ShowTimelineCommand = new DelegateCommand(_ => ShowTimeline(), _ => timelineWorkspace != null); |
There was a problem hiding this comment.
ShowTimelineCommand is created but is not bound anywhere in the WPF views, and the Compare workspace itself has no back action. After opening any snapshot, users therefore cannot return to Timeline to choose another snapshot without restarting the application or starting an unrelated backup, which also makes the implemented restore-set discard confirmation unreachable through the normal UI.
Useful? React with 👍 / 👎.
| internal Task RefreshAsync(CancellationToken cancellationToken = default) | ||
| { | ||
| cancellationToken.ThrowIfCancellationRequested(); | ||
| IReadOnlyList<SnapshotEvent> snapshots = catalog.Read(); |
There was a problem hiding this comment.
Every Timeline load calls catalog.Read() synchronously on the WPF dispatcher. That call recursively enumerates and stats every file in every discovered snapshot to calculate sizes, including remembered external or network roots, so a normal collection of large snapshots can prevent the window from painting or responding for a long time each time Timeline is revisited. Perform discovery on a worker thread and marshal only the resulting collection update back to the dispatcher.
Useful? React with 👍 / 👎.
Codex P1: - WpfThemeService: marshal system-theme reapply onto the application dispatcher (SystemEvents fires off the UI thread; resource-dictionary swaps there can terminate the process). Mirrors the WinForms handler. - BackupFolders: a bad default root no longer aborts the whole multi-root scan; custom/external roots are still enumerated so their backups remain restorable. The reason surfaces only when no root was readable. Tests (Claude): - Restore AppStoreApps.Verify ladder coverage (bad exit code, no file, empty file, no Packages array, invalid JSON) and the good-export path. - Restore Failed-not-Skipped coverage for Restore (no dialog registered, null owner). OpenCode: - Declare Newtonsoft.Json explicitly in WinRestoreKit.Application (AppRestoreService uses it directly; was arriving only transitively). - Timeline/AdvancedHistory ViewModels now inherit ObservableObject like every other ViewModel (made internal sealed to match the siblings and satisfy base-class accessibility). 986 passed, 0 failed, 0 skipped; Release build 0 warnings, 0 errors.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef1b4f60c1
ℹ️ 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".
| ShellViewModel shell = new ShellViewModel(themes, updates, | ||
| VersionInfo.GetCurrentVersion(typeof(App).Assembly)); |
There was a problem hiding this comment.
The WPF project does not set or link the product's AssemblyFileVersion, so the SDK gives WinRestoreKit.Wpf the default 1.0.0 while the canonical WinForms assembly is currently 0.0.1. Passing the WPF assembly here consequently makes About display 1.0.0, and UpdateCheckService.Decide treats release tags below that value as older, suppressing every normal 0.0.x update. Read the shared canonical version or apply the same version metadata to the WPF executable.
Useful? React with 👍 / 👎.
| else | ||
| { | ||
| usableArtifact = true; | ||
| artifactSummary = "The module verified a captured artifact."; |
There was a problem hiding this comment.
When a manifest exists but has no entry for this module, a positive artifact probe reaches this branch and marks the artifact usable. Two backups made in one process reuse the same Data.NowShort directory; if an earlier uncompressed run selected the module and a later run omits it, its loose artifact remains while the new manifest excludes it. Compare then enables restoring stale data from the earlier run, contrary to the manifest for the selected snapshot. Treat any module absent from a present manifest as not captured and reserve artifact probing for legacy manifestless backups.
Useful? React with 👍 / 👎.
| xmlns:views="clr-namespace:WinRestoreKit.Wpf.Views" | ||
| xmlns:vm="clr-namespace:WinRestoreKit.Wpf.ViewModels" | ||
| xmlns:timeline="clr-namespace:WinRestoreKit.Wpf.ViewModels.Timeline" | ||
| Title="WinRestoreKit" MinWidth="1024" MinHeight="640" |
There was a problem hiding this comment.
When a backup or restore is active, closing this sole WPF window still uses WPF's default last-window shutdown behavior; there is no Closing handler here or elsewhere in the shell to cancel or defer closure based on RunCoordinator.IsRunning. Clicking the title-bar close button therefore terminates the process in the middle of file or registry work, bypassing the run's orderly cancellation and cleanup and potentially leaving a partial restore or an incomplete backup. Prevent shutdown during a run or request cancellation and wait for the operation to settle.
Useful? React with 👍 / 👎.
1 participant
Summary
Implements the approved Timeline + Comparison Workspace design as a new WPF application running side-by-side with the existing WinForms shell. Both share the same Core engine and a new
WinRestoreKit.Applicationlibrary.What shipped
New projects
WinRestoreKit.Application— orchestration, backup presets, scope groups, snapshot event catalog, comparison service, update/theme/settings services extracted from WinForms and shared by both shellsWinRestoreKit.Wpf— Timeline home, Compare/Confirm restore workflow, Backup workspace, Progress, Results, Advanced History, Settings, About, App Restore dialogVisual system
ErrorCircleenum text at source)Crash fixes
Two read-only property bindings caused
InvalidOperationExceptionon view render:Run.TextCurrentVersion(read-only)Mode=OneWayProgressBar.ValuePercent(read-only)Mode=OneWayBoth have regression tests that render the view through the layout dispatcher.
Verification
WinRestoreKit.Wpf.exelaunches and renders elevatedNot in scope
publish-*/directories are gitignored (self-contained executables, ~70 MB each)