Conversation
Co-authored-by: Dara Adedeji <SunkenInTime@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryThis PR removes the custom
Confidence Score: 1/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant DesktopUpdateDialog
participant OldService as WindowsDesktopUpdateRestartService (removed)
participant Controller as DesktopUpdaterController
participant PS as PowerShell Script (removed)
User->>DesktopUpdateDialog: clicks "Restart to update"
DesktopUpdateDialog->>DesktopUpdateDialog: show confirmation dialog
Note over DesktopUpdateDialog,PS: OLD flow (Windows)
DesktopUpdateDialog->>OldService: restartIntoDownloadedUpdate()
OldService->>OldService: resolve executablePath + updateDirectory
OldService->>PS: write & launch detached .ps1 script
PS-->>PS: wait for process exit
PS-->>PS: Copy-Item update/* → installDir
PS-->>PS: Start-Process new executable
OldService->>DesktopUpdateDialog: exit(0)
Note over DesktopUpdateDialog,Controller: NEW flow (all platforms)
DesktopUpdateDialog->>Controller: restartApp()
Controller-->>User: restart (no file copy on Windows)
Last reviewed commit: "revert: restore orig..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (confirmed) { | ||
| try { | ||
| await WindowsDesktopUpdateRestartService() | ||
| .restartIntoDownloadedUpdate(); | ||
| } catch (error, stackTrace) { | ||
| AppErrorReporter.reportError( | ||
| 'Failed to apply the downloaded desktop update. Please close and reopen Icarus, then try again.', | ||
| error: error, | ||
| stackTrace: stackTrace, | ||
| source: 'DesktopUpdateDialog.restartToUpdate', | ||
| ); | ||
| controller.makeSkipUpdate(); | ||
| if (!context.mounted) { | ||
| return; | ||
| } | ||
| Navigator.of(context).pop(); | ||
| } | ||
| controller.restartApp(); | ||
| } |
There was a problem hiding this comment.
The original code called WindowsDesktopUpdateRestartService().restartIntoDownloadedUpdate(), which on Windows specifically wrote a detached PowerShell script to: (1) wait for the current process to terminate, (2) copy all files from the update/ subdirectory into the install directory, and (3) launch the new executable. On non-Windows, it fell through to _updater.restartApp().
The replacement controller.restartApp() is equivalent only to the non-Windows path. On Windows, it will restart the app without first copying the downloaded update files from update/ into the install directory, meaning the user will be running the same version they just "updated from". The desktop_updater package's restartApp() API does not perform this file-copy step — that was entirely custom to this repo.
Unless the desktop_updater package was recently updated to handle Windows file-copying natively (which would warrant a comment), Windows users will silently get a restart with no update applied.
| if (confirmed) { | ||
| try { | ||
| await WindowsDesktopUpdateRestartService() | ||
| .restartIntoDownloadedUpdate(); | ||
| } catch (error, stackTrace) { | ||
| AppErrorReporter.reportError( | ||
| 'Failed to apply the downloaded desktop update. Please close and reopen Icarus, then try again.', | ||
| error: error, | ||
| stackTrace: stackTrace, | ||
| source: 'DesktopUpdateDialog.restartToUpdate', | ||
| ); | ||
| controller.makeSkipUpdate(); | ||
| if (!context.mounted) { | ||
| return; | ||
| } | ||
| Navigator.of(context).pop(); | ||
| } | ||
| controller.restartApp(); | ||
| } |
There was a problem hiding this comment.
The previous implementation wrapped the restart call in a try/catch block that reported the failure via AppErrorReporter, called controller.makeSkipUpdate() to reset state, and popped the dialog so the user wasn't left in a broken UI state. The new single-line call has no error handling at all. If controller.restartApp() throws (e.g., the underlying platform channel fails), the exception will be unhandled, the dialog will remain open with no feedback, and the user won't know the restart failed.
Consider wrapping in a try/catch:
if (confirmed) {
try {
controller.restartApp();
} catch (error, stackTrace) {
// report error and reset state
controller.makeSkipUpdate();
if (!context.mounted) return;
Navigator.of(context).pop();
}
}There was a problem hiding this comment.
Pull request overview
This PR removes the custom Windows “restart into downloaded update” implementation and shifts the update-install restart flow to rely solely on desktop_updater’s DesktopUpdaterController.restartApp().
Changes:
- Remove
WindowsDesktopUpdateRestartService(PowerShell-based apply+restart flow on Windows). - Update
DesktopUpdateDialogto callcontroller.restartApp()directly when an update is downloaded. - Delete the unit test that validated the generated Windows restart script.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| test/windows_desktop_update_restart_service_test.dart | Deleted test that covered the Windows restart/apply PowerShell script behavior. |
| lib/widgets/desktop_update_dialog.dart | Replaces custom restart/apply + error reporting with a direct controller.restartApp() call. |
| lib/services/windows_desktop_update_restart_service.dart | Removes the Windows-specific update apply/restart service entirely. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (confirmed) { | ||
| try { | ||
| await WindowsDesktopUpdateRestartService() | ||
| .restartIntoDownloadedUpdate(); | ||
| } catch (error, stackTrace) { | ||
| AppErrorReporter.reportError( | ||
| 'Failed to apply the downloaded desktop update. Please close and reopen Icarus, then try again.', | ||
| error: error, | ||
| stackTrace: stackTrace, | ||
| source: 'DesktopUpdateDialog.restartToUpdate', | ||
| ); | ||
| controller.makeSkipUpdate(); | ||
| if (!context.mounted) { | ||
| return; | ||
| } | ||
| Navigator.of(context).pop(); | ||
| } | ||
| controller.restartApp(); | ||
| } |
There was a problem hiding this comment.
PR metadata says this is a docs-only marker with no behavior changes, but this hunk changes update-install behavior by removing the custom Windows restart/apply flow and switching to controller.restartApp(). Please update the PR title/description to reflect the functional change (or restore the prior behavior if this is meant to be a throwaway docs-only diff).
| } | ||
| Navigator.of(context).pop(); | ||
| } | ||
| controller.restartApp(); |
There was a problem hiding this comment.
This removes the previous try/catch + AppErrorReporter path around applying the downloaded update. If controller.restartApp() throws/fails, the user will get no actionable feedback and the dialog state (skipUpdate/pop) won't be updated. Consider reintroducing error reporting + fallback behavior (e.g., reportError with a source string and call controller.makeSkipUpdate()/pop) around the restart call.
| controller.restartApp(); | |
| try { | |
| await controller.restartApp(); | |
| } catch (error, stackTrace) { | |
| FlutterError.reportError( | |
| FlutterErrorDetails( | |
| exception: error, | |
| stack: stackTrace, | |
| library: 'desktop_update_dialog', | |
| context: ErrorDescription( | |
| 'while restarting app after applying a downloaded update', | |
| ), | |
| ), | |
| ); | |
| controller.makeSkipUpdate(); | |
| if (!mounted) return; | |
| Navigator.of(context).pop(); | |
| } |
| controller.restartApp(); | ||
| } |
There was a problem hiding this comment.
This change removes the only automated coverage around the Windows update restart/apply path (the deleted restart-script unit test) and replaces it with a direct controller.restartApp() call. Please add a new test covering the updated behavior (e.g., a widget test that the "Restart to update" action calls controller.restartApp(), and any error/fallback handling if reintroduced).
Summary
Testing
Notes