Conversation
WalkthroughThe changes introduce deferred installer execution capabilities to the version update workflow. A new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant VersionWindow
participant ExternalApplication
participant VersionFolder as VersionFolder Helper
participant Downloader
participant RunLocalFile
User->>VersionWindow: Click "Update Now"
VersionWindow->>VersionFolder: Call VersionFolder()
VersionFolder-->>VersionWindow: Return path
VersionWindow->>Downloader: Download installer
Downloader-->>VersionWindow: Installer file location
VersionWindow->>RunLocalFile: Action(fileLocation)
RunLocalFile->>RunLocalFile: Execute installer immediately
User->>VersionWindow: Click "Update on Exit"
VersionWindow->>VersionFolder: Call VersionFolder()
VersionFolder-->>VersionWindow: Return path
VersionWindow->>Downloader: Download installer
Downloader-->>VersionWindow: Installer file location
VersionWindow->>ExternalApplication: Set InstallerRoute
VersionWindow->>VersionWindow: Close window
Note over ExternalApplication: On application shutdown
ExternalApplication->>RunLocalFile: Action(InstallerRoute)
RunLocalFile->>RunLocalFile: Execute installer deferred
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Common_glTF_Exporter/Windows/VersionWindow.xaml.cs (2)
29-52: Eliminate code duplication between update handlers.Both
Button_ClickAsyncandButton_UpdateOnExitcontain nearly identical download logic. Only the final action differs.Extract common download logic:
+private async Task<string> DownloadInstaller() +{ + string versionFolder = VersionFolder(); + await DownloadFile.FromServer(versionFolder, _installerName); + return System.IO.Path.Combine(versionFolder, _installerName); +} + private async void Button_ClickAsync(object sender, RoutedEventArgs e) { this.Hide(); - - string versionFolder = VersionFolder(); - await DownloadFile.FromServer(versionFolder, _installerName); - - string fileLocation = System.IO.Path.Combine(versionFolder, _installerName);; + string fileLocation = await DownloadInstaller(); RunLocalFile.Action(fileLocation); - Close(); } private async void Button_UpdateOnExit(object sender, RoutedEventArgs e) { this.Hide(); - string versionFolder = VersionFolder(); - await DownloadFile.FromServer(versionFolder, _installerName); - - string fileLocation = System.IO.Path.Combine(versionFolder, _installerName); + string fileLocation = await DownloadInstaller(); ExternalApplication.InstallerRoute = fileLocation; Close(); }
29-52: Add error handling to async button handlers.Both update handlers lack try-catch blocks. If the download fails (network error, server unavailable, etc.), the user receives no feedback and the window state becomes inconsistent.
Wrap async operations in try-catch:
private async void Button_ClickAsync(object sender, RoutedEventArgs e) { - this.Hide(); - string versionFolder = VersionFolder(); - await DownloadFile.FromServer(versionFolder, _installerName); - - string fileLocation = System.IO.Path.Combine(versionFolder, _installerName);; - RunLocalFile.Action(fileLocation); - - Close(); + try + { + this.Hide(); + string versionFolder = VersionFolder(); + await DownloadFile.FromServer(versionFolder, _installerName); + + string fileLocation = System.IO.Path.Combine(versionFolder, _installerName); + RunLocalFile.Action(fileLocation); + Close(); + } + catch (Exception ex) + { + this.Show(); + TaskDialog.Show("Update Error", + $"Failed to download or launch installer: {ex.Message}"); + } }Apply similar error handling to
Button_UpdateOnExit.
🧹 Nitpick comments (2)
Common_glTF_Exporter/ExternalApplication.cs (1)
29-29: Consider renaming toInstallerPathfor clarity.The term "Path" is more conventional than "Route" when referring to file system locations.
Apply this diff:
- public static string InstallerRoute; + public static string InstallerPath;Note: This would also require updating references in
VersionWindow.xaml.cs.Common_glTF_Exporter/Windows/VersionWindow.xaml (1)
118-134: Verify the intentional styling difference between the two buttons.The "Update on Exit" button explicitly sets
Background="Transparent"andForeground="{DynamicResource WindowButtonColor}"(lines 123-124), while the "Update Now" button relies on theMainButtonStyledefaults. If this difference is intentional to establish visual hierarchy (secondary vs. primary action), consider adding a comment to document this design choice. Otherwise, ensure consistent styling between the buttons.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Common_glTF_Exporter/ExternalApplication.cs(3 hunks)Common_glTF_Exporter/Windows/VersionWindow.xaml(1 hunks)Common_glTF_Exporter/Windows/VersionWindow.xaml.cs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
Common_glTF_Exporter/ExternalApplication.cs (1)
Common_glTF_Exporter/Version/RunLocalFile.cs (2)
RunLocalFile(6-15)Action(8-14)
Common_glTF_Exporter/Windows/VersionWindow.xaml.cs (3)
Common_glTF_Exporter/Version/DownloadFile.cs (1)
DownloadFile(13-124)Common_glTF_Exporter/ExternalApplication.cs (1)
ExternalApplication(20-198)Common_glTF_Exporter/Utils/DirectoryUtils.cs (3)
DirectoryUtils(5-42)CreateDirectoryIfNotExists(10-21)DeleteFilesInDirectoyy(34-41)
🔇 Additional comments (3)
Common_glTF_Exporter/Windows/VersionWindow.xaml.cs (1)
33-40: Good refactoring to use theVersionFolder()helper.The extraction of folder management logic improves code reusability.
Common_glTF_Exporter/Windows/VersionWindow.xaml (2)
23-24: LGTM: Enter key binding maps to primary action.The Enter key binding (lines 23-24) correctly triggers the "Open" command, which executes
Button_ClickAsyncfor the "Update Now" button. This provides appropriate keyboard accessibility for the primary update action.Also applies to: 31-31, 130-130
114-137: LGTM: Well-structured two-button layout.The horizontal StackPanel with centered alignment provides a clean layout for the two update options. The consistent button sizing (100x30) and margins (10,0,10,13) create a balanced visual appearance suitable for a dialog window.
| if (!string.IsNullOrEmpty(InstallerRoute)) | ||
| { | ||
| RunLocalFile.Action(InstallerRoute); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Add error handling and consider timing implications.
The installer execution on shutdown has several concerns:
- Timing: Launching the installer immediately during shutdown may fail if Revit hasn't fully released file locks on its assemblies.
- No error handling: If
RunLocalFile.Actionfails silently, users won't know why the update didn't occur. - State cleanup: The
InstallerRoutefield is never cleared, which could cause issues if checked in future sessions.
Consider this improved implementation:
public Result OnShutdown(UIControlledApplication application)
{
if (!string.IsNullOrEmpty(InstallerRoute))
{
- RunLocalFile.Action(InstallerRoute);
+ try
+ {
+ RunLocalFile.Action(InstallerRoute);
+ }
+ catch (Exception ex)
+ {
+ TaskDialog.Show("Update Error",
+ $"Failed to launch installer: {ex.Message}");
+ }
+ finally
+ {
+ InstallerRoute = null;
+ }
}
return Result.Succeeded;
}Verify that the installer can successfully launch while Revit is shutting down by testing on multiple machines with different configurations.
🏁 Script executed:
#!/bin/bash
# Find and inspect ExternalApplication.cs file
fd -t f -i "ExternalApplication.cs" --exec cat {} \;Length of output: 9126
🏁 Script executed:
#!/bin/bash
# Search for RunLocalFile class and its Action method
rg -nP "class\s+RunLocalFile|\.Action\s*\(" --type=cs -A 5Length of output: 1270
🏁 Script executed:
#!/bin/bash
# Find all uses of InstallerRoute field
rg -nP "InstallerRoute" --type=cs -B 3 -A 3Length of output: 2098
🏁 Script executed:
#!/bin/bash
# Search for error handling patterns in shutdown/plugin code
rg -nP "OnShutdown|try\s*\{" --type=cs -A 10 | head -80Length of output: 1837
🏁 Script executed:
#!/bin/bash
# Look for TaskDialog usage (showing how errors are reported elsewhere)
rg -nP "TaskDialog\." --type=cs -B 2 -A 2 | head -60Length of output: 527
🏁 Script executed:
#!/bin/bash
# Get full RunLocalFile.cs implementation
fd -t f "RunLocalFile.cs" --exec cat {} \;Length of output: 323
🏁 Script executed:
#!/bin/bash
# Check VersionWindow context - when/how InstallerRoute is set
rg -nP "VersionWindow" --type=cs -A 10 -B 5 | head -100Length of output: 3216
🏁 Script executed:
#!/bin/bash
# Look for Revit shutdown documentation references or comments about OnShutdown
rg -nP "OnShutdown|shutdown|Revit.*release|file.*lock" --type=cs -i | head -40Length of output: 4984
🏁 Script executed:
#!/bin/bash
# Check IExternalApplication contract and OnShutdown requirements
rg -nP "IExternalApplication|Result.Succeeded" --type=cs -A 3 -B 1 | head -50Length of output: 2239
Add error handling and clear field after use.
The installer execution on shutdown lacks proper error handling and state cleanup:
- Missing error handling:
RunLocalFile.ActioncallsHyperlink.Run()without catching exceptions. If the installer fails to launch, the error goes unreported. - No state cleanup: The static
InstallerRoutefield is never cleared after use, which could cause unintended behavior in subsequent sessions.
Wrap the call in try-catch-finally to handle failures gracefully and clear the field:
public Result OnShutdown(UIControlledApplication application)
{
if (!string.IsNullOrEmpty(InstallerRoute))
{
- RunLocalFile.Action(InstallerRoute);
+ try
+ {
+ RunLocalFile.Action(InstallerRoute);
+ }
+ catch (Exception ex)
+ {
+ TaskDialog.Show("Update Error",
+ $"Failed to launch installer: {ex.Message}");
+ }
+ finally
+ {
+ InstallerRoute = null;
+ }
}
return Result.Succeeded;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!string.IsNullOrEmpty(InstallerRoute)) | |
| { | |
| RunLocalFile.Action(InstallerRoute); | |
| } | |
| if (!string.IsNullOrEmpty(InstallerRoute)) | |
| { | |
| try | |
| { | |
| RunLocalFile.Action(InstallerRoute); | |
| } | |
| catch (Exception ex) | |
| { | |
| TaskDialog.Show("Update Error", | |
| $"Failed to launch installer: {ex.Message}"); | |
| } | |
| finally | |
| { | |
| InstallerRoute = null; | |
| } | |
| } |
🤖 Prompt for AI Agents
In Common_glTF_Exporter/ExternalApplication.cs around lines 62 to 65, the
installer launch lacks exception handling and never clears the static
InstallerRoute; wrap the RunLocalFile.Action(InstallerRoute) call in a
try-catch-finally: in try call RunLocalFile.Action, in catch log or report the
exception (include exception message and context) so failures are visible, and
in finally clear the InstallerRoute field (set to null or empty) to remove state
after use.
| private async void Button_UpdateOnExit(object sender, RoutedEventArgs e) | ||
| { | ||
| this.Hide(); | ||
| string versionFolder = VersionFolder(); | ||
| await DownloadFile.FromServer(versionFolder, _installerName); | ||
|
|
||
| string fileLocation = System.IO.Path.Combine(versionFolder, _installerName); | ||
| ExternalApplication.InstallerRoute = fileLocation; | ||
| Close(); | ||
| } |
There was a problem hiding this comment.
Critical timing issue: Window closes before download completes.
The window is hidden immediately (line 45) and then closed (line 51), but the download is awaited in between (line 47). This creates several problems:
- User confusion: No feedback that download is happening in background
- Race condition: If user closes Revit before download completes, installer won't be available to run on shutdown
- Silent failures: If download fails, user receives no notification
Consider one of these approaches:
Option 1 (Recommended): Keep window visible with progress feedback:
private async void Button_UpdateOnExit(object sender, RoutedEventArgs e)
{
- this.Hide();
+ labelMessage.Text = "Downloading installer...";
+ CloseButton.IsEnabled = false;
string versionFolder = VersionFolder();
await DownloadFile.FromServer(versionFolder, _installerName);
string fileLocation = System.IO.Path.Combine(versionFolder, _installerName);
ExternalApplication.InstallerRoute = fileLocation;
+ labelMessage.Text = "Update will install when you close Revit.";
+ await Task.Delay(2000); // Brief confirmation
Close();
}Option 2: Start download as fire-and-forget background task (not recommended due to reliability concerns):
private void Button_UpdateOnExit(object sender, RoutedEventArgs e)
{
string versionFolder = VersionFolder();
- await DownloadFile.FromServer(versionFolder, _installerName);
-
- string fileLocation = System.IO.Path.Combine(versionFolder, _installerName);
- ExternalApplication.InstallerRoute = fileLocation;
+ Task.Run(async () => {
+ await DownloadFile.FromServer(versionFolder, _installerName);
+ string fileLocation = System.IO.Path.Combine(versionFolder, _installerName);
+ ExternalApplication.InstallerRoute = fileLocation;
+ });
Close();
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In Common_glTF_Exporter/Windows/VersionWindow.xaml.cs around lines 43 to 52, the
window is hidden immediately and then closed while awaiting the download which
can confuse users and cause race conditions; change the flow to keep the window
visible and block close until download completes by disabling the window
controls (or showing a modal progress dialog), start the awaited download with
progress reporting, update a progress UI element as bytes arrive, handle
exceptions from DownloadFile.FromServer to show a user-facing error message (and
do not set ExternalApplication.InstallerRoute or close the window on failure),
and only set ExternalApplication.InstallerRoute and close the window after a
successful download completes; ensure UI updates are marshaled to the UI thread
and the Close/Hide calls occur after success.
| private static string VersionFolder() | ||
| { | ||
| string tempPath = Path.GetTempPath(); | ||
| string tempFolder = System.IO.Path.Combine(tempPath, "LeiaInstaller"); | ||
| string versionFolder = System.IO.Path.Combine(tempFolder, _fileVersion); | ||
| DirectoryUtils.CreateDirectoryIfNotExists(versionFolder); | ||
| DirectoryUtils.DeleteFilesInDirectoyy(versionFolder); | ||
| return versionFolder; | ||
| } |
There was a problem hiding this comment.
Add error handling for directory operations.
The VersionFolder method performs file system operations without error handling. If directory creation or file deletion fails (e.g., due to permissions or locked files), the exception will bubble up unhandled.
Add try-catch to provide graceful error handling:
private static string VersionFolder()
{
- string tempPath = Path.GetTempPath();
- string tempFolder = System.IO.Path.Combine(tempPath, "LeiaInstaller");
- string versionFolder = System.IO.Path.Combine(tempFolder, _fileVersion);
- DirectoryUtils.CreateDirectoryIfNotExists(versionFolder);
- DirectoryUtils.DeleteFilesInDirectoyy(versionFolder);
- return versionFolder;
+ try
+ {
+ string tempPath = Path.GetTempPath();
+ string tempFolder = System.IO.Path.Combine(tempPath, "LeiaInstaller");
+ string versionFolder = System.IO.Path.Combine(tempFolder, _fileVersion);
+ DirectoryUtils.CreateDirectoryIfNotExists(versionFolder);
+ DirectoryUtils.DeleteFilesInDirectoyy(versionFolder);
+ return versionFolder;
+ }
+ catch (Exception ex)
+ {
+ TaskDialog.Show("Update Error",
+ $"Failed to prepare installer directory: {ex.Message}");
+ throw;
+ }
}Note: There's a typo in DirectoryUtils.DeleteFilesInDirectoyy (should be "Directory"), but that's outside this PR's scope.
Committable suggestion skipped: line range outside the PR's diff.
1 participant
Summary by CodeRabbit