Add New API for ProgressBoxEx to Show Progress & Add Progress Display for Plugin Downloading & Improve DownloadUrl Api Function #3170
Conversation
This comment has been minimized.
This comment has been minimized.
|
🥷 Code experts: jjw24 jjw24 has most 🧠 knowledge in the files. See details
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame: To learn more about /:\ gitStream - Visit our Docs |
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
📝 WalkthroughWalkthroughThis pull request introduces a new method, Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (15)
Plugins/Flow.Launcher.Plugin.PluginsManager/PluginsManager.cs (10)
145-146: Consider using aCancellationTokenSourcefor download cancellation.
Currently,downloadCancelledis a simple boolean. If future requirements call for more complex scenarios (like timeouts or multiple parallel downloads), aCancellationTokenSourcemay be more robust. For now, this approach is straightforward and acceptable.
154-156: Add timeouts or retry policy to the HttpClient usage.
UsingHttpClientwithout an explicit timeout or retry logic might cause the application to hang indefinitely if the server fails to respond. Adding a timeout or retry policy could improve reliability.
157-157: Log failed responses.
response.EnsureSuccessStatusCode()is good. However, if it fails, consider logging relevant headers or status codes for better diagnostics.
159-161: Support indefinite progress when content length is unknown.
IfContentLengthis-1, no progress reporting occurs. Consider a fallback indefinite progress bar for such situations to signal ongoing activity to the user.
170-189: Check chunk size and iteration performance.
The usage of an 8KB buffer inside awhileloop is typical. For very large downloads, you might consider measuring performance or adjusting the buffer size to reduce overhead. Also, consider verifying cancellation more frequently if extremely large files must be handled.
191-201: Combine download methods to reduce duplication.
IfcanReportProgressisfalse, the code defaults toHttp.DownloadAsync. That is fine, but you might unify logic for progress/no-progress scenarios to maintain consistent error handling or support partial progress.
211-213: Consider post-cancellation cleanup.
You return immediately ifdownloadCancelledistrue, skipping installation. If partial files exist atfilePath, consider cleaning them up to avoid leaving orphaned files.
219-234: Show additional diagnostics onHttpRequestException.
Currently, the code logs a general error and displays a generic message. Consider including the HTTP status code or partial stack trace to help with troubleshooting.
239-254: Consolidate code in exception handlers.
The logic to close the progress box and display an error is repeated betweenHttpRequestExceptionand the generalExceptionblock. Consider extracting this into a helper method to reduce duplication.
Line range hint
420-492: Add overall progress for “Update All” operation.
When updating multiple plugins, it may be helpful to display a combined progress indicator. This would align the user experience with single plugin updates by showing progress for each or overall.Flow.Launcher.Core/ProgressBoxEx.xaml.cs (3)
14-18: Constructor approach is straightforward.
InvokingInitializeComponent()inside the constructor is standard. Ensure that_forceClosedis not null-checked here if you plan to handle edge cases later inForceClose.
44-65: Dispatcher invocation inReportProgress.
Propagating method calls to the main UI thread is essential for WPF UI changes. The value clamping to 0 or 100 is correct. If you anticipate extremely large increments, consider floating-point rounding.
78-82: Esc key closure is user-friendly.
Binding the Escape key is a nice usability feature. Ensure you communicate this behavior to users in any relevant documentation, if needed.Flow.Launcher.Core/ProgressBoxEx.xaml (2)
1-16: Window sizing and theming.
TheWidth="420"withSizeToContent="Height"and dynamic resources for background/foreground ensure a flexible UI that can adapt to theming changes. Confirm that 420px is sufficient for localized text.
26-62: Title bar close button approach.
The custom path and styling for the close button is user-friendly and matches typical “x” icons. Check that the path dimensions remain consistent on high-DPI systems.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Flow.Launcher.Core/ProgressBoxEx.xaml(1 hunks)Flow.Launcher.Core/ProgressBoxEx.xaml.cs(1 hunks)Flow.Launcher.Plugin/IProgressBoxEx.cs(1 hunks)Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs(2 hunks)Flow.Launcher/PublicAPIInstance.cs(1 hunks)Plugins/Flow.Launcher.Plugin.PluginsManager/Main.cs(1 hunks)Plugins/Flow.Launcher.Plugin.PluginsManager/PluginsManager.cs(6 hunks)
🔇 Additional comments (15)
Plugins/Flow.Launcher.Plugin.PluginsManager/PluginsManager.cs (3)
162-168: Validate usage of the cancellation callback.
The cancellation callback setsdownloadCancelledandprgBox = null. Ensure that any subsequent logic that referencesprgBoxhandles the null state gracefully.
407-407: Ensure the user is notified on failed tasks.
UsingContinueWith(..., TaskContinuationOptions.OnlyOnFaulted, ...)is valid for cleanup or logging. Confirm that the user receives a clear error message or UI feedback if an update fails in a background task.
628-628: Method name is consistent and clear.
RenamingRequestInstallOrUpdatetoRequestInstallOrUpdateAsyncclarifies its asynchronous nature, aligning with best practices.Flow.Launcher.Plugin/IProgressBoxEx.cs (1)
1-20: Interface design appears cohesive.
DefiningReportProgress(double progress)andClose()suffices for most simple progress-box use cases. In future expansions, consider adding asynchronous variants, advanced cancellation, or status text updates if needed.Plugins/Flow.Launcher.Plugin.PluginsManager/Main.cs (1)
54-54: Good adaptation of asynchronous API usage.
ReplacingRequestInstallOrUpdatewithRequestInstallOrUpdateAsyncensures consistency and proper async handling in the query method. No issues noted.Flow.Launcher.Core/ProgressBoxEx.xaml.cs (5)
9-13: Interface implementation and field usage.
ProgressBoxExcorrectly implementsIProgressBoxEx. The_forceClosedfield is stored for a callback, and_isClosedensures we don't close multiple times. This is a well-structured approach.
20-42: Robust UI-thread checks & error handling inShowmethod.
CallingShowvia the dispatcher ensures thread safety. The try-catch block logs exceptions without crashing the application. This approach is commendable for user-facing components.
67-77: Safe override ofClosemethod.
The_isClosedguard ensures the window is not double-closed, preventing potential exceptions. This pattern is good for UI stability.
83-91: Cancel button event.
ForceCloseis consistently called for both the “Cancel” button and the close button, avoiding duplicated logic. This helps reduce complexity.
93-104:ForceClosehandles the callback.
Invoking_forceClosed?.Invoke()after verifying_isClosedis a good pattern. This ensures that external cleanup or cancellation logic is triggered exactly once.Flow.Launcher.Core/ProgressBoxEx.xaml (3)
17-25: WindowChrome and keyboard shortcuts.
DefiningWindowChromewithCaptionHeight="32"provides a modern look. UsingKeyBindingforEscape→Closeis an excellent usability enhancement.
63-86: Title and progress bar arrangement.
Placing theTextBlockandProgressBarin separate rows is a clean layout. The margins are well-balanced. If a very long title is shown, ensure the text wrapping doesn't overlap the progress bar.
87-105: Footer container and Cancel button.
Using aBorderfor the footer with a separate button region is a nice separation of concerns in the UI. The use of dynamic resources for background/border color ensures theming consistency.Flow.Launcher/PublicAPIInstance.cs (1)
327-328: Consider thread-safety measures and explicit documentation.This new method is straightforward, but confirm whether it's safe to call on threads other than the UI thread. If the call must be confined to the UI thread, explicitly mention that in documentation or throw an appropriate exception when invoked from a background thread. This approach can help prevent race conditions and GUI operation errors.
Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs (1)
319-327: Ensure consistency and clarify null return conditions.The new
ShowProgressBoxmethod aligns well with the existing "ShowX" naming style. However, the documentation mentions a possible null return; verify if calling this method in certain edge conditions or from non-UI threads might produce a null result. If so, add clarifications to help plugin developers handle this safely.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
Plugins/Flow.Launcher.Plugin.PluginsManager/PluginsManager.cs (1)
Line range hint
424-496: Add progress reporting and cancellation support for batch updates.The "Update All" functionality lacks progress reporting and cancellation support, which are available for single plugin updates.
Consider implementing:
- Progress reporting for batch updates
- Cancellation support using the provided CancellationToken
- Parallel download limits to prevent overwhelming the server
Example implementation available upon request.
🧹 Nitpick comments (1)
Plugins/Flow.Launcher.Plugin.PluginsManager/PluginsManager.cs (1)
162-172: Simplify progress box initialization and disposal.The progress box initialization and disposal logic can be simplified using pattern matching and null-coalescing operators.
- if (canReportProgress && - (prgBox = Context.API.ShowProgressBox(prgBoxTitle, () => - { - if (prgBox != null) - { - httpClient.CancelPendingRequests(); - downloadCancelled = true; - prgBox = null; - } - })) != null) + if (canReportProgress && + Context.API.ShowProgressBox(prgBoxTitle, () => + { + httpClient.CancelPendingRequests(); + downloadCancelled = true; + }) is IProgressBoxEx progress) + { + prgBox = progress;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Plugins/Flow.Launcher.Plugin.PluginsManager/PluginsManager.cs(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: gitStream.cm
🔇 Additional comments (2)
Plugins/Flow.Launcher.Plugin.PluginsManager/PluginsManager.cs (2)
632-632: LGTM! Method naming follows async conventions.The method rename to include the 'Async' suffix follows C# naming conventions for asynchronous methods.
181-193: 🛠️ Refactor suggestionAdd ConfigureAwait(false) to async operations.
Missing ConfigureAwait(false) in async operations can lead to potential deadlocks in UI applications.
- while ((read = await contentStream.ReadAsync(buffer).ConfigureAwait(false)) > 0) + while ((read = await contentStream.ReadAsync(buffer).ConfigureAwait(false)) > 0) { - await fileStream.WriteAsync(buffer.AsMemory(0, read)).ConfigureAwait(false); + await fileStream.WriteAsync(buffer.AsMemory(0, read)).ConfigureAwait(false); totalRead += read; var progressValue = totalRead * 100 / totalBytes;Likely invalid or redundant comment.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Mind sharing some screenshot (or a short video) to demonstrate the behavior? |
Sure, please check updated desc. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
I have considered to use delegate to report the progress. (This indeed can remove the IProgressBoxEx) But in So I need to get the progress window in the exception sentence, like using |
|
I don't really understand sorry. Why does delegate cannot handle that? I think we can use some special value (like -1) to represent error on the delegate so I don't really think that's a problem? |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
Flow.Launcher/ProgressBoxEx.xaml.cs (1)
111-113:⚠️ Potential issueAvoid hiding base class method
Close()The method
private void Close()hides the inheritedClose()method from theWindowclass. Hiding methods can lead to confusion and maintenance issues.As per previous discussions and learned guidelines, consider renaming the method to avoid hiding the base class method:
-private void Close() +private void ForceCloseWindow() { Close(); _forceClosed?.Invoke(); }Update all references to
Close()within this class toForceCloseWindow().
🧹 Nitpick comments (4)
Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs (2)
319-332: Ensure consistent documentation and method orderingThe documentation for
ShowMsgBoxandShowProgressBoxAsyncshould be consistent in style and detail. Additionally, consider placing the newShowProgressBoxAsyncmethod appropriately within the interface for better readability.
- Align the documentation style with existing methods.
- Place the new method after related message box methods.
330-331: Clarify the return value in the documentationThe summary for
ShowProgressBoxAsyncmentions that it returns a progress box interface, but the method returns aTask. Clarify the return type and its purpose.Update the method documentation to accurately reflect the return value.
/// <returns>A task that represents the asynchronous operation.</returns>Plugins/Flow.Launcher.Plugin.PluginsManager/PluginsManager.cs (2)
222-237: Consolidate exception handling and loggingThe exception handling in the catch blocks is similar. To reduce code duplication and improve clarity, consider consolidating them.
Extract common code into a method or adjust the catch blocks accordingly.
Line range hint
417-475: Consider usingMessageBoxResult.Yesfor consistencyIn the
AsyncActionfor the "Update All" result, you check forMessageBoxResult.No, whereas in other places you check forMessageBoxResult.Yes. This inconsistency can lead to confusion.Modify the condition to check for
MessageBoxResult.Yesto align with other parts of the code.if (Context.API.ShowMsgBox(message, Context.API.GetTranslation("plugin_pluginsmanager_update_title"), - MessageBoxButton.YesNo) == MessageBoxResult.No) + MessageBoxButton.YesNo) != MessageBoxResult.Yes) { return false; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs(2 hunks)Flow.Launcher/ProgressBoxEx.xaml.cs(1 hunks)Flow.Launcher/PublicAPIInstance.cs(1 hunks)Plugins/Flow.Launcher.Plugin.PluginsManager/PluginsManager.cs(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Flow.Launcher/PublicAPIInstance.cs
🧰 Additional context used
📓 Learnings (1)
Flow.Launcher/ProgressBoxEx.xaml.cs (1)
Learnt from: taooceros
PR: Flow-Launcher/Flow.Launcher#3170
File: Flow.Launcher/ProgressBoxEx.xaml.cs:79-88
Timestamp: 2025-01-10T04:58:20.225Z
Learning: In Flow.Launcher, avoid hiding base class methods with the `new` keyword. Instead, use event handlers, method overriding (where the base method is virtual), or create new methods with different names to extend functionality.
🔇 Additional comments (5)
Flow.Launcher/ProgressBoxEx.xaml.cs (2)
13-17: 🛠️ Refactor suggestionCall
InitializeComponent()before other initializationIt's a good practice to call
InitializeComponent()at the beginning of the constructor to ensure that all UI components are initialized before being accessed.Apply this diff to reorder the constructor:
private ProgressBoxEx(Action forceClosed) { + InitializeComponent(); _forceClosed = forceClosed; - InitializeComponent(); }Likely invalid or redundant comment.
60-81: Prevent multiple progress reports after closureIf
ReportProgressis called after the progress box is closed, it may result in exceptions.Ensure that
ReportProgresscannot be called after the progress box has been closed. Consider adding a guard condition to check if the window is still open before updating the progress.private void ReportProgress(double progress) { if (!Application.Current.Dispatcher.CheckAccess()) { Application.Current.Dispatcher.Invoke(() => ReportProgress(progress)); return; } + if (!IsVisible) + return; + if (progress < 0) { ProgressBar.Value = 0; } else if (progress >= 100) { ProgressBar.Value = 100; Close(); } else { ProgressBar.Value = progress; } }✅ Verification successful
The suggested guard condition is necessary and correct
The
IsVisiblecheck is important because:
- The progress window is used in async operations that may continue reporting progress after the window is closed
- Without the guard, there's a risk of accessing disposed UI controls when progress updates arrive after closure
- The solution follows WPF best practices for handling window lifecycle in async scenarios
The suggested implementation should be applied as proposed in the review comment.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Find usages of ProgressBoxEx rg "ProgressBoxEx" -l # Look for instantiation and usage patterns rg "new ProgressBoxEx" -A 5 # Find similar progress reporting patterns ast-grep --pattern 'ReportProgress($$$)'Length of output: 802
Script:
#!/bin/bash # Check PublicAPIInstance.cs content cat Flow.Launcher/PublicAPIInstance.cs # Look for async patterns around ProgressBox rg -B 3 -A 10 "ProgressBoxEx" --type csLength of output: 17378
Plugins/Flow.Launcher.Plugin.PluginsManager/PluginsManager.cs (3)
20-20: ReuseHttpClientcarefully to avoid socket exhaustionUsing a static
HttpClientinstance is generally recommended, but care must be taken to manage its lifetime and avoid unintended consequences.Ensure that the static
HttpClientis properly configured and that default settings likeTimeoutare appropriate for all use cases.
191-194:⚠️ Potential issueEnsure progress box is closed upon cancellation
When
downloadCancelledistrue, the method returns without closing the progress box, which may leave it open.Modify the code to close the progress box before returning:
if (downloadCancelled) { + await Context.API.HideProgressBoxAsync(); return; }Assuming there is a method to hide or close the progress box. If not, ensure the progress box is properly closed in this scenario.
Likely invalid or redundant comment.
147-203: 🛠️ Refactor suggestionHandle cancellation tokens properly during download
The cancellation token
cts.Tokenshould be passed to all asynchronous methods that support it, includingReadAsyncandWriteAsync, to ensure that the operation can be cancelled promptly.Apply this diff to include the cancellation token:
while ((read = await contentStream.ReadAsync(buffer, cts.Token).ConfigureAwait(false)) > 0) { await fileStream.WriteAsync(buffer.AsMemory(0, read), cts.Token).ConfigureAwait(false); totalRead += read; var progressValue = totalRead * 100 / totalBytes; if (downloadCancelled) return; else reportProgress(progressValue); }Likely invalid or redundant comment.
This comment has been minimized.
This comment has been minimized.
@taooceros Sorry, it seems that I encountered the thread issue last time. Now update and please check. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
Plugins/Flow.Launcher.Plugin.PluginsManager/PluginsManager.cs (4)
20-21: Use HttpClientFactory instead of static HttpClient.While using a static HttpClient is better than creating new instances per request, consider injecting IHttpClientFactory for better lifecycle management and resilience.
-private static readonly HttpClient HttpClient = new(); +private readonly IHttpClientFactory _httpClientFactory; +public PluginsManager(PluginInitContext context, Settings settings, IHttpClientFactory httpClientFactory) +{ + Context = context; + Settings = settings; + _httpClientFactory = httpClientFactory; +}
170-176: Fix typo in comment.There's a typo in the comment: "expcetion" should be "exception".
- // when reportProgress is null, it means there is expcetion with the progress box + // when reportProgress is null, it means there is exception with the progress box
207-211: Consider using exponential backoff for redownload.When redownloading after an exception, consider implementing exponential backoff to handle temporary network issues.
if (exceptionHappened && (!downloadCancelled)) - await Http.DownloadAsync(plugin.UrlDownload, filePath).ConfigureAwait(false); +{ + var policy = Policy + .Handle<HttpRequestException>() + .WaitAndRetryAsync(3, retryAttempt => + TimeSpan.FromSeconds(Math.Pow(2, retryAttempt))); + + await policy.ExecuteAsync(async () => + await Http.DownloadAsync(plugin.UrlDownload, filePath).ConfigureAwait(false)); +}
Line range hint
411-483: Consider chunking parallel updates.When updating multiple plugins in parallel, consider chunking them into smaller batches to avoid overwhelming the system.
-await Task.WhenAll(resultsForUpdate.Select(async plugin => +const int batchSize = 3; +var pluginBatches = resultsForUpdate + .Select((plugin, index) => new { plugin, index }) + .GroupBy(x => x.index / batchSize) + .Select(g => g.Select(x => x.plugin)); + +foreach (var batch in pluginBatches) +{ + await Task.WhenAll(batch.Select(async plugin =>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Plugins/Flow.Launcher.Plugin.PluginsManager/PluginsManager.cs(7 hunks)
🔇 Additional comments (5)
Plugins/Flow.Launcher.Plugin.PluginsManager/PluginsManager.cs (5)
147-148: LGTM: Proper cancellation handling.Good implementation of cancellation using CancellationTokenSource and download cancellation flag.
Also applies to: 156-157
161-163: LGTM: Robust content length handling.Good practice to handle cases where content length is not available (-1).
222-226: LGTM: Proper cancellation check before installation.Good practice to check for cancellation before proceeding with installation.
619-619: LGTM: Method renamed to reflect async nature.Good practice to append 'Async' suffix to asynchronous methods.
186-189: 🛠️ Refactor suggestionPass CancellationToken to async operations.
ReadAsync and WriteAsync operations should use the cancellation token for proper cancellation support.
-while ((read = await contentStream.ReadAsync(buffer).ConfigureAwait(false)) > 0) -{ - await fileStream.WriteAsync(buffer.AsMemory(0, read)).ConfigureAwait(false); +while ((read = await contentStream.ReadAsync(buffer, cts.Token).ConfigureAwait(false)) > 0) +{ + await fileStream.WriteAsync(buffer.AsMemory(0, read), cts.Token).ConfigureAwait(false);Likely invalid or redundant comment.
Totally understand the thread issue. Maybe you can make the ProgressBar MVVM so we didn't modify the UI state directly but rather updating the view via the property (which by WPF spec is thread safe)? |
|
As the stackoverflow indicated, could you make the ProgressReport part an extension method of Http (maybe inside the Http class). That would also allow other parts of code to utilize this feature if they wants. Then interact the progress report with the ProgressWindow via delegate or something instead of handling the stream itself, which makes this part of code too complicated. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
I see. I improve the |
Jack251970
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
Flow.Launcher.Infrastructure/Http/Http.cs (5)
102-106: Consider using a larger buffer size for better performance.The current 8KB buffer size might not be optimal for large downloads. Consider increasing it to 81920 (80KB) which is commonly used for file operations.
- var buffer = new byte[8192]; + var buffer = new byte[81920];
114-120: Potential for progress reporting optimization.The current implementation reports progress on every chunk read, which could be excessive for fast downloads. Consider adding a throttle to reduce UI updates.
+ const double progressThreshold = 1.0; // Report only 1% changes progressValue = totalRead * 100.0 / totalBytes; + var newProgress = Math.Floor(progressValue); + if (Math.Abs(newProgress - lastReportedProgress) >= progressThreshold) + { if (token.IsCancellationRequested) return; else - reportProgress(progressValue); + { + reportProgress(newProgress); + lastReportedProgress = newProgress; + } + }
122-123: Ensure final progress is reported only if not already at 100%.The current implementation might unnecessarily report 100% progress even if it was already reported in the last chunk.
- if (progressValue < 100) + if (Math.Floor(progressValue) < 100) reportProgress(100);
127-128: Add buffer size parameter to CopyToAsync for consistency.When falling back to the non-progress path, specify the same buffer size for consistency with the progress reporting path.
- await response.Content.CopyToAsync(fileStream, token); + await response.Content.CopyToAsync(fileStream, token, 81920);
86-129: Consider implementing download resume capability.For large plugin downloads, it would be beneficial to support resuming interrupted downloads. This could be implemented by:
- Checking for existing partial downloads
- Using HTTP Range headers
- Appending to existing files when possible
Would you like me to provide a detailed implementation for this feature?
Plugins/Flow.Launcher.Plugin.PluginsManager/PluginsManager.cs (2)
172-175: Consider adding a retry limit.The current implementation retries the download once if an exception occurs. Consider adding a retry limit and exponential backoff to prevent infinite retries.
+private const int MaxRetries = 3; +private static readonly TimeSpan InitialRetryDelay = TimeSpan.FromSeconds(1); // if exception happened while downloading and user does not cancel downloading, // we need to redownload the plugin -if (exceptionHappened && (!cts.IsCancellationRequested)) - await Http.DownloadAsync(plugin.UrlDownload, filePath, null, cts.Token).ConfigureAwait(false); +if (exceptionHappened && (!cts.IsCancellationRequested)) +{ + for (int retry = 0; retry < MaxRetries; retry++) + { + try + { + await Task.Delay(TimeSpan.FromSeconds(Math.Pow(2, retry)) * InitialRetryDelay, cts.Token); + await Http.DownloadAsync(plugin.UrlDownload, filePath, null, cts.Token).ConfigureAwait(false); + break; + } + catch (Exception) when (retry < MaxRetries - 1) + { + continue; + } + } +}
190-207: Consider adding more specific exception handling.The current implementation catches general exceptions which might mask specific issues. Consider adding separate catch blocks for common exceptions like
IOException,UnauthorizedAccessException, etc.catch (HttpRequestException e) { Context.API.ShowMsgError( string.Format(Context.API.GetTranslation("plugin_pluginsmanager_downloading_plugin"), plugin.Name), Context.API.GetTranslation("plugin_pluginsmanager_download_error")); Log.Exception("PluginsManager", "An error occurred while downloading plugin", e); return; } +catch (IOException e) +{ + Context.API.ShowMsgError( + Context.API.GetTranslation("plugin_pluginsmanager_install_error_title"), + string.Format(Context.API.GetTranslation("plugin_pluginsmanager_io_error"), plugin.Name)); + Log.Exception("PluginsManager", "An I/O error occurred while handling plugin", e); + return; +} +catch (UnauthorizedAccessException e) +{ + Context.API.ShowMsgError( + Context.API.GetTranslation("plugin_pluginsmanager_install_error_title"), + string.Format(Context.API.GetTranslation("plugin_pluginsmanager_access_error"), plugin.Name)); + Log.Exception("PluginsManager", "Access denied while handling plugin", e); + return; +} catch (Exception e) { Context.API.ShowMsgError( Context.API.GetTranslation("plugin_pluginsmanager_install_error_title"), string.Format(Context.API.GetTranslation("plugin_pluginsmanager_install_error_subtitle"), plugin.Name)); Log.Exception("PluginsManager", "An error occurred while downloading plugin", e); return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Flow.Launcher.Core/Plugin/JsonRPCV2Models/JsonRPCPublicAPI.cs(1 hunks)Flow.Launcher.Infrastructure/Http/Http.cs(1 hunks)Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs(3 hunks)Flow.Launcher/ProgressBoxEx.xaml.cs(1 hunks)Flow.Launcher/PublicAPIInstance.cs(2 hunks)Plugins/Flow.Launcher.Plugin.PluginsManager/PluginsManager.cs(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- Flow.Launcher/PublicAPIInstance.cs
- Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs
- Flow.Launcher/ProgressBoxEx.xaml.cs
🔇 Additional comments (4)
Flow.Launcher.Core/Plugin/JsonRPCV2Models/JsonRPCPublicAPI.cs (1)
123-126: LGTM! Clean implementation of progress reporting.The updated
HttpDownloadAsyncmethod signature properly integrates progress reporting while maintaining backward compatibility with the optionalreportProgressparameter.Flow.Launcher.Infrastructure/Http/Http.cs (1)
94-96: Good practice checking content length availability.The check for content length availability before attempting progress reporting helps prevent issues with servers that don't provide this information.
Plugins/Flow.Launcher.Plugin.PluginsManager/PluginsManager.cs (2)
20-20: LGTM! Improved encapsulation.Making the
zipconstant private properly restricts its access scope to this class.
579-579: Verify all callers of the renamed method.The method has been renamed from
RequestInstallOrUpdatetoRequestInstallOrUpdateAsyncto better reflect its asynchronous nature. This is a breaking change that requires verification of all callers.✅ Verification successful
Method rename verification completed successfully
All callers have been updated to use the new async method name. The method is internal and has no interface or inheritance dependencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining calls to the old method name rg "RequestInstallOrUpdate[^A]" --type csLength of output: 43
Script:
#!/bin/bash # Search for the new async method name usage rg "RequestInstallOrUpdateAsync" --type cs -A 2 # Check if this is part of an interface or has derived classes ast-grep --pattern 'interface $_ { $$$ RequestInstallOrUpdate$$$ }' # Look for class inheritance ast-grep --pattern 'class $_ : PluginsManager { $$$ }'Length of output: 1062
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Flow.Launcher.Infrastructure/Http/Http.cs (2)
86-95: Consider usingIProgress<T>instead ofAction<double>.While
Action<double>works,IProgress<T>is the recommended pattern for progress reporting in .NET as it:
- Handles thread synchronization automatically
- Provides better error handling
- Supports cancellation more elegantly
- Is more testable
- public static async Task DownloadAsync([NotNull] string url, [NotNull] string filePath, Action<double> reportProgress = null, CancellationToken token = default) + public static async Task DownloadAsync([NotNull] string url, [NotNull] string filePath, IProgress<double> progress = null, CancellationToken token = default)
97-124: Optimize progress reporting frequency.The current implementation reports progress after every chunk, which could lead to excessive UI updates for large files. Consider implementing a throttling mechanism to reduce the frequency of progress updates.
if (canReportProgress && reportProgress != null) { await using var contentStream = await response.Content.ReadAsStreamAsync(token); await using var fileStream = new FileStream(filePath, FileMode.CreateNew, FileAccess.Write, FileShare.None, 8192, true); var buffer = new byte[8192]; long totalRead = 0; int read; double progressValue = 0; + double lastReportedProgress = 0; + const double progressThreshold = 1.0; // Report only when progress changes by 1% reportProgress(0); while ((read = await contentStream.ReadAsync(buffer, token)) > 0) { await fileStream.WriteAsync(buffer.AsMemory(0, read), token); totalRead += read; progressValue = totalRead * 100.0 / totalBytes; if (token.IsCancellationRequested) return; - else + else if (progressValue - lastReportedProgress >= progressThreshold) + { reportProgress(progressValue); + lastReportedProgress = progressValue; + } } if (progressValue < 100) reportProgress(100); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher.Infrastructure/Http/Http.cs(1 hunks)
🔇 Additional comments (1)
Flow.Launcher.Infrastructure/Http/Http.cs (1)
125-129: LGTM! Consistent file handling between progress and non-progress paths.The implementation correctly uses
FileMode.CreateNewin both paths, addressing the previous review comment about FileMode inconsistency.
This comment has been minimized.
This comment has been minimized.
@check-spelling-bot Report🔴 Please reviewSee the 📂 files view, the 📜action log, or 📝 job summary for details.
See ❌ Event descriptions for more information. If the flagged items are 🤯 false positivesIf items relate to a ...
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
Plugins/Flow.Launcher.Plugin.PluginsManager/PluginsManager.cs (4)
147-164: Enhance cancellation handling with proper cleanup.While the cancellation token implementation is good, we should ensure proper cleanup of resources when cancellation is requested.
if (cts.IsCancellationRequested) +{ + if (File.Exists(filePath)) + File.Delete(filePath); return; +}
168-183: Improve error handling with specific exception types.The error handling is good but could be more specific. Consider catching
UnauthorizedAccessExceptionseparately for file system operations.+catch (UnauthorizedAccessException e) +{ + Context.API.ShowMsgError( + Context.API.GetTranslation("plugin_pluginsmanager_install_error_title"), + Context.API.GetTranslation("plugin_pluginsmanager_install_error_access_denied")); + Log.Exception("PluginsManager", "Access denied while downloading plugin", e); + return; +} catch (Exception e)
202-228: Consider retry mechanism for failed downloads.The method handles exceptions well but could benefit from a retry mechanism for transient network issues.
private async Task DeleteFileAndDownloadMsgBoxAsync(string prgBoxTitle, string downloadUrl, string filePath, CancellationTokenSource cts) { + const int maxRetries = 3; + const int delayMs = 1000; + if (File.Exists(filePath)) File.Delete(filePath); var exceptionHappened = false; + var retryCount = 0; + + while (retryCount < maxRetries) + { await Context.API.ShowProgressBoxAsync(prgBoxTitle, async (reportProgress) => { if (reportProgress == null) { exceptionHappened = true; return; } else { await Context.API.HttpDownloadAsync(downloadUrl, filePath, reportProgress, cts.Token).ConfigureAwait(false); } }, cts.Cancel); if (!exceptionHappened || cts.IsCancellationRequested) break; + retryCount++; + if (retryCount < maxRetries) + await Task.Delay(delayMs * retryCount, cts.Token); + } - if (exceptionHappened && (!cts.IsCancellationRequested)) - await Context.API.HttpDownloadAsync(downloadUrl, filePath).ConfigureAwait(false); }
419-430: Consider parallel download limit for update all.When updating multiple plugins simultaneously, consider implementing a semaphore to limit parallel downloads.
+private static SemaphoreSlim _downloadSemaphore = new SemaphoreSlim(3); // Limit to 3 concurrent downloads await Task.WhenAll(resultsForUpdate.Select(async plugin => { var downloadToFilePath = Path.Combine(Path.GetTempPath(), $"{plugin.Name}-{plugin.NewVersion}.zip"); try { + await _downloadSemaphore.WaitAsync(); using var cts = new CancellationTokenSource(); await DeleteFileAndDownloadMsgBoxAsync(...); + _downloadSemaphore.Release(); if (cts.IsCancellationRequested) return;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Plugins/Flow.Launcher.Plugin.PluginsManager/PluginsManager.cs(10 hunks)
🔇 Additional comments (3)
Plugins/Flow.Launcher.Plugin.PluginsManager/PluginsManager.cs (3)
20-20: LGTM! Access modifier change improves encapsulation.The change from
consttoprivate constfor thezipvariable appropriately restricts access.
317-358: LGTM! Good implementation of progress reporting and cancellation.The update functionality properly handles progress reporting and cancellation.
369-369:⚠️ Potential issueHandle potential null exception in continuation task.
The continuation task assumes
t.Exception.InnerExceptionis not null, which might not always be true.-Log.Exception("PluginsManager", $"Update failed for {x.Name}", t.Exception.InnerException); +Log.Exception("PluginsManager", $"Update failed for {x.Name}", t.Exception?.InnerException ?? t.Exception);Likely invalid or redundant comment.
Content
IProgressBoxEx ShowProgressBox(string caption, Action forceClosed = null).Screenshots
Interface
Cancel Download
2025-01-09.16-39-48.mp4
Finish Download
2025-01-09.16-40-07.mp4
Test
ProgressBoxExusage in PluginManager plugin (progress displaying & cancellation)