-
-
Notifications
You must be signed in to change notification settings - Fork 443
Catch scary exception, print friendly log #3975
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
Catch scary exception, print friendly log #3975
Conversation
Rather than showing the user `EXCEPTION OCCURS: System.Threading.Tasks.TaskCanceledException...`, print not scary log msg
🥷 Code experts: Jack251970 Jack251970 has most 👩💻 activity in the files. See details
Activity based on git-commit:
Knowledge based on git-blame: ✨ Comment |
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
📝 WalkthroughWalkthroughAdds explicit cancellation handling in FetchAsync of CommunityPluginSource: catches OperationCanceledException (when token requested) and TaskCanceledException, logs and returns null; existing HTTP-status handling and generic exception catch remain unchanged. No public API changes. (≈34 words) Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant CommunityPluginSource as CommunityPluginSource.FetchAsync
participant HttpClient
Caller->>CommunityPluginSource: FetchAsync(ct)
CommunityPluginSource->>HttpClient: send request (with ct)
alt Request completes
HttpClient-->>CommunityPluginSource: Response (200/304/other)
CommunityPluginSource-->>Caller: result per status handling
else Cancelled by token
HttpClient-->>CommunityPluginSource: throws OperationCanceledException
rect #E8F5E9
Note right of CommunityPluginSource: Catch OperationCanceledException\nLog info\nReturn null
end
CommunityPluginSource-->>Caller: null
else HttpClient timeout or external cancel
HttpClient-->>CommunityPluginSource: throws TaskCanceledException
rect #FFF3E0
Note right of CommunityPluginSource: Catch TaskCanceledException\nLog warn\nReturn null
end
CommunityPluginSource-->>Caller: null
else Other error
HttpClient-->>CommunityPluginSource: throws Exception
rect #FFEBEE
Note right of CommunityPluginSource: Generic catch\nLog error\nHandle/return null
end
CommunityPluginSource-->>Caller: null or error result
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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: 1
🧹 Nitpick comments (2)
Flow.Launcher.Core/ExternalPlugins/CommunityPluginSource.cs (2)
46-49
: Dispose request and avoid sending an empty If-None-Match headerMinor cleanup: dispose HttpRequestMessage and only add the ETag header when present to prevent invalid/empty header values.
- var request = new HttpRequestMessage(HttpMethod.Get, ManifestFileUrl); - - request.Headers.Add("If-None-Match", latestEtag); + using var request = new HttpRequestMessage(HttpMethod.Get, ManifestFileUrl); + if (!string.IsNullOrWhiteSpace(latestEtag)) + request.Headers.Add("If-None-Match", latestEtag);
57-63
: Guard against null deserialization result to prevent NREReadFromJsonAsync can return null; using plugins.Count immediately after may throw. Default to an empty list.
- plugins = await response.Content - .ReadFromJsonAsync<List<UserPlugin>>(PluginStoreItemSerializationOption, cancellationToken: token) - .ConfigureAwait(false); + var deserialized = await response.Content + .ReadFromJsonAsync<List<UserPlugin>>(PluginStoreItemSerializationOption, cancellationToken: token) + .ConfigureAwait(false); + plugins = deserialized ?? new List<UserPlugin>();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher.Core/ExternalPlugins/CommunityPluginSource.cs
(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3572
File: Flow.Launcher/App.xaml.cs:214-216
Timestamp: 2025-07-06T12:21:37.947Z
Learning: In Flow Launcher, the UpdatePluginManifestAsync method in PluginsManifest.cs already has comprehensive internal try-catch handling that logs exceptions and returns false on failure rather than throwing, making external try-catch wrappers unnecessary.
📚 Learning: 2025-07-06T12:21:37.947Z
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3572
File: Flow.Launcher/App.xaml.cs:214-216
Timestamp: 2025-07-06T12:21:37.947Z
Learning: In Flow Launcher, the UpdatePluginManifestAsync method in PluginsManifest.cs already has comprehensive internal try-catch handling that logs exceptions and returns false on failure rather than throwing, making external try-catch wrappers unnecessary.
Applied to files:
Flow.Launcher.Core/ExternalPlugins/CommunityPluginSource.cs
📚 Learning: 2025-07-21T09:19:49.684Z
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3854
File: Flow.Launcher/App.xaml.cs:246-262
Timestamp: 2025-07-21T09:19:49.684Z
Learning: In Flow Launcher's App.xaml.cs, the asynchronous plugin initialization task (containing AbstractPluginEnvironment.PreStartPluginExecutablePathUpdate, PluginManager.LoadPlugins, PluginManager.InitializePluginsAsync, and AutoPluginUpdates) does not require additional try-catch error handling according to maintainer Jack251970, as these operations are designed to handle exceptions internally.
Applied to files:
Flow.Launcher.Core/ExternalPlugins/CommunityPluginSource.cs
📚 Learning: 2025-08-13T06:12:43.382Z
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3897
File: Flow.Launcher/ViewModel/PluginViewModel.cs:46-51
Timestamp: 2025-08-13T06:12:43.382Z
Learning: In Flow Launcher's PluginViewModel.cs, the LoadIconAsync method does not require additional try-catch error handling according to maintainer Jack251970, as the existing error handling approach is considered sufficient for the image loading operations.
Applied to files:
Flow.Launcher.Core/ExternalPlugins/CommunityPluginSource.cs
📚 Learning: 2025-07-21T09:19:19.012Z
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3854
File: Flow.Launcher.Core/Plugin/PluginManager.cs:280-292
Timestamp: 2025-07-21T09:19:19.012Z
Learning: In Flow Launcher's PluginManager.cs, the post-initialization operations (RegisterResultsUpdatedEvent, UpdatePluginMetadataTranslation, RegisterPluginActionKeywords, DialogJump.InitializeDialogJumpPlugin, and AddPluginToLists) are designed to be exception-safe and do not require additional try-catch error handling according to the maintainer Jack251970.
Applied to files:
Flow.Launcher.Core/ExternalPlugins/CommunityPluginSource.cs
⏰ 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). (2)
- GitHub Check: gitStream.cm
- GitHub Check: build
…b.com/dcog989/Flow.Launcher into CommunityPluginSource-logging-refined
@dcog989 Thanks for your contribution! |
…efined Catch scary exception, print friendly log
Rather than showing the user
EXCEPTION OCCURS: System.Threading.Tasks.TaskCanceledException...
, print not scary log msg