-
Notifications
You must be signed in to change notification settings - Fork 293
UI: Improve Upgrade Available Experience #347
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
When clicking the upgrade notification, shows a dialog with: - Current and new version information - Release notes from Velopack (NotesHTML) - Upgrade and Ignore buttons User can review release notes before deciding to upgrade. Closes #344
WalkthroughAdds an interactive upgrade confirmation flow: MainViewModel now shows an UpgradeDialog (view + viewmodel) with current/new versions and release notes (WebView2). The dialog returns an explicit user choice (Upgrade or Ignore) which MainViewModel respects before continuing the download/apply sequence. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant MVM as MainViewModel
participant WM as WindowManager
participant UDVM as UpgradeDialogViewModel
participant UDV as UpgradeDialogView
participant WebView as WebView2
User->>MVM: Trigger upgrade flow
activate MVM
MVM->>MVM: Retrieve release notes & versions
MVM->>UDVM: Create with current/new/releaseNotes
MVM->>WM: Show UDVM (modal)
activate WM
WM->>UDV: Instantiate & display dialog
activate UDV
UDV->>WebView: Initialize & Navigate(ReleaseNotesHtml)
WebView-->>UDV: Loaded / Error
User->>UDV: Click Upgrade or Ignore
UDV->>UDVM: Invoke Upgrade()/Ignore()
UDVM-->>MVM: Return user choice
alt choice == Upgrade
MVM->>MVM: Log and continue download/apply update
else choice == Ignore
MVM->>MVM: Log and return early
end
deactivate MVM
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Papercut.UI/ViewModels/UpgradeDialogViewModel.cs (1)
68-78: Consider using consistent boolean parameter for TryCloseAsync.Both methods set
UserChoicecorrectly, butUpgrade()passestruetoTryCloseAsyncwhileIgnore()passesfalse. Since theUserChoiceproperty already tracks the user's decision, consider using the same boolean value for consistency unless there's a specific reason for the difference.If the boolean parameter doesn't affect downstream logic, consider this refactor:
public void Upgrade() { this.UserChoice = UpgradeChoice.Upgrade; this.TryCloseAsync(true); } public void Ignore() { this.UserChoice = UpgradeChoice.Ignore; - this.TryCloseAsync(false); + this.TryCloseAsync(true); }src/Papercut.UI/ViewModels/MainViewModel.cs (1)
492-514: Consider using injected IViewModelWindowManager for consistency.The code creates a new
WindowManagerinstance manually (line 503), but this file already uses_viewModelWindowManagerfor other dialogs (see lines 570, 604, 664). Using the injected window manager ensures consistent dialog behavior and respects any application-wide window manager configuration.Apply this change to use the existing pattern:
- // Get the window manager from DI and show the dialog manually - var windowManager = new Caliburn.Micro.WindowManager(); - var dialogResult = await windowManager.ShowDialogAsync(upgradeDialog); + // Show the dialog using the injected window manager + var dialogResult = await this._viewModelWindowManager.ShowDialogAsync(upgradeDialog);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Papercut.UI/ViewModels/MainViewModel.cs(1 hunks)src/Papercut.UI/ViewModels/UpgradeDialogViewModel.cs(1 hunks)src/Papercut.UI/Views/UpgradeDialogView.xaml(1 hunks)src/Papercut.UI/Views/UpgradeDialogView.xaml.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/Papercut.UI/ViewModels/MainViewModel.cs (2)
src/Papercut.Common/Helper/AssemblyHelper.cs (1)
GetVersion(25-30)src/Papercut.UI/ViewModels/UpgradeDialogViewModel.cs (3)
UpgradeDialogViewModel(22-79)UpgradeDialogViewModel(24-58)Upgrade(68-72)
src/Papercut.UI/Views/UpgradeDialogView.xaml.cs (1)
src/Papercut.UI/ViewModels/UpgradeDialogViewModel.cs (2)
UpgradeDialogViewModel(22-79)UpgradeDialogViewModel(24-58)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Papercut SMTP
- GitHub Check: Analyze (csharp)
- GitHub Check: Build Papercut SMTP
🔇 Additional comments (4)
src/Papercut.UI/ViewModels/UpgradeDialogViewModel.cs (1)
24-58: LGTM! Clean constructor with appropriate fallback.The constructor properly handles null or empty release notes with a well-styled fallback HTML message. The fallback provides a good user experience when release notes are unavailable.
src/Papercut.UI/Views/UpgradeDialogView.xaml.cs (2)
31-57: Verify WebView2 runtime availability before showing dialog.The code initializes WebView2 without checking if the runtime is installed. While error handling is in place (lines 43-55), the user experience would be better if the dialog validates WebView2 availability before being displayed. The
MainViewModelhas access to_webView2Information.IsInstalled(line 136 in MainViewModel.cs) which could be used to check availability upstream.Consider adding a check in
MainViewModel.UpgradeToLatest()before creating the dialog:if (!this._webView2Information.IsInstalled) { await this.ShowMessageAsync("Update Information", "Release notes require WebView2. Please proceed with the upgrade to view full release information."); // Optionally skip dialog and proceed directly to upgrade }Alternatively, verify whether WebView2 is a required dependency for the application and if its absence should prevent showing the dialog entirely.
46-54: Error handling provides good fallback experience.The error HTML fallback properly uses
SecurityElement.Escape()to prevent potential XSS issues when displaying exception messages. This is a good defensive coding practice.src/Papercut.UI/Views/UpgradeDialogView.xaml (1)
1-88: LGTM! Clean XAML structure with proper MahApps styling.The view is well-structured with:
- Clear visual hierarchy (header, content, actions)
- Consistent use of MahApps.Metro theme resources
- Appropriate window properties (centered, resizable, not in taskbar)
- Caliburn.Micro convention-based button naming that correctly binds to view model methods
The design provides a professional upgrade experience consistent with the application's UI patterns.
Replace manual WindowManager instantiation with the injected _viewModelWindowManager for consistency with other dialogs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Papercut.UI/ViewModels/MainViewModel.cs (1)
503-503: Consider removing the unuseddialogResultvariable.The
dialogResultvariable is captured but never used. The user's choice is correctly checked viaupgradeDialog.UserChoiceinstead. For cleaner code, consider using a discard:- var dialogResult = await this._viewModelWindowManager.ShowDialogAsync(upgradeDialog); + _ = await this._viewModelWindowManager.ShowDialogAsync(upgradeDialog);Alternatively, you could simply await without capturing:
- var dialogResult = await this._viewModelWindowManager.ShowDialogAsync(upgradeDialog); + await this._viewModelWindowManager.ShowDialogAsync(upgradeDialog);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Papercut.UI/ViewModels/MainViewModel.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Papercut.UI/ViewModels/MainViewModel.cs (2)
src/Papercut.Common/Helper/AssemblyHelper.cs (1)
GetVersion(25-30)src/Papercut.UI/ViewModels/UpgradeDialogViewModel.cs (3)
UpgradeDialogViewModel(22-79)UpgradeDialogViewModel(24-58)Upgrade(68-72)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Papercut SMTP
- GitHub Check: Analyze (csharp)
- GitHub Check: Build Papercut SMTP
🔇 Additional comments (2)
src/Papercut.UI/ViewModels/MainViewModel.cs (2)
506-512: LGTM! User choice handling is correct.The logic properly handles all cases:
- Aborts if user chose "Ignore" or closed the dialog without choosing (None)
- Logs the user's decision appropriately
- Only proceeds with upgrade when explicitly confirmed
492-500: No action required on Velopack API usage.The
NotesHTMLproperty exists onVelopackAssetand is properly documented in the official Velopack API. The version extraction logic, dialog creation, and release notes retrieval are all correct.
Summary
NotesHTMLpropertyChanges
UpgradeDialogViewModel.cs- ViewModel with version info and user choice trackingUpgradeDialogView.xaml- MahApps.Metro styled dialog with WebView2 for release notesUpgradeDialogView.xaml.cs- Code-behind for WebView2 initializationMainViewModel.UpgradeToLatest()- Shows dialog before proceeding with downloadTest plan
Closes #344
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.