Handle MinimumAppVersion parse fail and reorder last manifest fetch date #3943
Conversation
|
🥷 Code experts: Jack251970, taooceros 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. |
There was a problem hiding this comment.
Pull Request Overview
This PR improves error handling for plugin manifest processing by adding exception handling for version parsing and optimizes the timing of when the manifest fetch timestamp is recorded.
- Adds try-catch block around MinimumAppVersion parsing to handle invalid version strings gracefully
- Moves the lastFetchedAt timestamp update to occur after successful processing instead of at the beginning
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughDefers updating lastFetchedAt until after successfully processing and assigning fetched plugins; refactors IsMinimumAppVersionSatisfied to handle empty values, parse failures (try/catch with exception logging), and explicit version-mismatch logging and boolean returns. No public API signatures changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant App as App
participant PM as PluginsManifest
participant Repo as PluginRepo
User->>App: request manifest update
App->>PM: UpdateManifestAsync()
PM->>Repo: fetch manifests
Repo-->>PM: results (empty / items / error)
alt results empty
PM-->>App: return (no update)
else results present
PM->>PM: build updatedPluginResults
PM->>PM: UserPlugins = updatedPluginResults
PM->>PM: lastFetchedAt = now
PM-->>App: return success
end
sequenceDiagram
autonumber
participant PM as PluginsManifest
participant Logger
PM->>PM: IsMinimumAppVersionSatisfied(minVer, appVer)
alt minVer null/empty
PM-->>PM: return true
else minVer provided
PM->>PM: try parse minVer
alt parse fails
PM->>Logger: Log exception with plugin info
PM-->>PM: return false
else parse succeeds
alt appVer >= minVer
PM-->>PM: return true
else appVer < minVer
PM->>Logger: Log info about required minimum
PM-->>PM: return false
end
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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 (1)
Flow.Launcher.Core/ExternalPlugins/PluginsManifest.cs (1)
76-90: Harden MinimumAppVersion parsing against whitespace and reduce noisy failures.Trim and treat whitespace-only values as “unset” to avoid excluding valid plugins due to formatting. Keeps the new behavior while reducing exception spam.
Apply this diff:
- if (string.IsNullOrEmpty(plugin.MinimumAppVersion)) + if (string.IsNullOrWhiteSpace(plugin.MinimumAppVersion)) return true; try { - if (appVersion >= SemanticVersioning.Version.Parse(plugin.MinimumAppVersion)) + var minimum = plugin.MinimumAppVersion.Trim(); + if (appVersion >= SemanticVersioning.Version.Parse(minimum)) return true; } catch (Exception e) { - API.LogException(ClassName, $"Failed to parse the minimum app version {plugin.MinimumAppVersion} for plugin {plugin.Name}. " + API.LogException(ClassName, $"Failed to parse the minimum app version '{plugin.MinimumAppVersion}' for plugin {plugin.Name}. " + "Plugin excluded from manifest", e); return false; }Additionally (outside the changed hunk), consider trimming the app version when parsing to be defensive:
// line ~47 var appVersion = SemanticVersioning.Version.Parse(Constant.Version.Trim());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
Flow.Launcher.Core/ExternalPlugins/PluginsManifest.cs(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 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.
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3791
File: Flow.Launcher.Core/Plugin/PluginManager.cs:293-295
Timestamp: 2025-07-01T05:46:13.251Z
Learning: In Flow.Launcher.Core/Plugin/PluginManager.cs, when checking if a plugin is modified within the PluginManager class itself, prefer using the internal static PluginModified(string id) method directly rather than going through API.PluginModified() for better performance and architectural design.
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.
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.
Learnt from: taooceros
PR: Flow-Launcher/Flow.Launcher#2616
File: Flow.Launcher/Flow.Launcher.csproj:7-7
Timestamp: 2024-10-08T15:52:58.573Z
Learning: In the Flow Launcher project, the version number in the `Flow.Launcher.csproj` file is dynamically updated during the CI/CD process.
📚 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/PluginsManifest.cs
📚 Learning: 2025-07-01T05:46:13.251Z
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3791
File: Flow.Launcher.Core/Plugin/PluginManager.cs:293-295
Timestamp: 2025-07-01T05:46:13.251Z
Learning: In Flow.Launcher.Core/Plugin/PluginManager.cs, when checking if a plugin is modified within the PluginManager class itself, prefer using the internal static PluginModified(string id) method directly rather than going through API.PluginModified() for better performance and architectural design.
Applied to files:
Flow.Launcher.Core/ExternalPlugins/PluginsManifest.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/PluginsManifest.cs
🧬 Code graph analysis (1)
Flow.Launcher.Core/ExternalPlugins/PluginsManifest.cs (2)
Plugins/Flow.Launcher.Plugin.WindowsSettings/Log.cs (1)
Exception(15-18)Plugins/Flow.Launcher.Plugin.Explorer/ContextMenu.cs (1)
LogException(489-492)
⏰ 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
🔇 Additional comments (1)
Flow.Launcher.Core/ExternalPlugins/PluginsManifest.cs (1)
57-58: Good fix: update lastFetchedAt only after successful processing.This aligns with the PR goal and prevents stale-success timestamps when fetch returns empty or processing fails.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Flow.Launcher.Core/ExternalPlugins/PluginsManifest.cs (1)
34-36: Bug: unconditional Release() can over-increment semaphore on canceled wait.If WaitAsync throws (e.g., OperationCanceledException), finally still calls Release(), causing SemaphoreFullException and breaking future updates.
Minimal fix:
- try + var entered = false; + try { - await manifestUpdateLock.WaitAsync(token).ConfigureAwait(false); + await manifestUpdateLock.WaitAsync(token).ConfigureAwait(false); + entered = true; ... } finally { - manifestUpdateLock.Release(); + if (entered) + manifestUpdateLock.Release(); }Optionally handle OperationCanceledException separately to log at Debug/Info and return false gracefully.
Also applies to: 66-69
♻️ Duplicate comments (1)
Flow.Launcher.Core/ExternalPlugins/PluginsManifest.cs (1)
84-89: Catching generic Exception here is acceptable per prior discussion.Previous suggestion to narrow to specific exceptions was declined; the current approach is consistent with that decision.
🧹 Nitpick comments (2)
Flow.Launcher.Core/ExternalPlugins/PluginsManifest.cs (2)
38-38: Use UTC clock to avoid DST/clock-skew issues.Comparisons based on local time can behave oddly across DST jumps and manual clock changes. Prefer UtcNow for both the check and assignment.
Apply:
- if (UserPlugins == null || usePrimaryUrlOnly || DateTime.Now.Subtract(lastFetchedAt) >= fetchTimeout) + if (UserPlugins == null || usePrimaryUrlOnly || DateTime.UtcNow - lastFetchedAt >= fetchTimeout)- lastFetchedAt = DateTime.Now; + lastFetchedAt = DateTime.UtcNow;Also applies to: 57-57
76-77: Treat whitespace-only MinimumAppVersion as empty.Using IsNullOrWhiteSpace avoids needless parse attempts on " " values.
- if (string.IsNullOrEmpty(plugin.MinimumAppVersion)) + if (string.IsNullOrWhiteSpace(plugin.MinimumAppVersion)) return true;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
Flow.Launcher.Core/ExternalPlugins/PluginsManifest.cs(2 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: jjw24
PR: Flow-Launcher/Flow.Launcher#3932
File: Flow.Launcher.Core/ExternalPlugins/PluginsManifest.cs:48-55
Timestamp: 2025-09-04T11:52:29.074Z
Learning: In Flow Launcher's PluginsManifest.cs, when dealing with version parsing for the MinimumAppVersion feature, maintainer jjw24 prefers to keep the solution simple rather than implementing comprehensive helper methods for SemVer parsing normalization.
📚 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/PluginsManifest.cs
📚 Learning: 2025-09-04T11:52:29.074Z
Learnt from: jjw24
PR: Flow-Launcher/Flow.Launcher#3932
File: Flow.Launcher.Core/ExternalPlugins/PluginsManifest.cs:48-55
Timestamp: 2025-09-04T11:52:29.074Z
Learning: In Flow Launcher's PluginsManifest.cs, when dealing with version parsing for the MinimumAppVersion feature, maintainer jjw24 prefers to keep the solution simple rather than implementing comprehensive helper methods for SemVer parsing normalization.
Applied to files:
Flow.Launcher.Core/ExternalPlugins/PluginsManifest.cs
📚 Learning: 2025-07-01T05:46:13.251Z
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3791
File: Flow.Launcher.Core/Plugin/PluginManager.cs:293-295
Timestamp: 2025-07-01T05:46:13.251Z
Learning: In Flow.Launcher.Core/Plugin/PluginManager.cs, when checking if a plugin is modified within the PluginManager class itself, prefer using the internal static PluginModified(string id) method directly rather than going through API.PluginModified() for better performance and architectural design.
Applied to files:
Flow.Launcher.Core/ExternalPlugins/PluginsManifest.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/PluginsManifest.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
🔇 Additional comments (3)
Flow.Launcher.Core/ExternalPlugins/PluginsManifest.cs (3)
57-58: Good move: update lastFetchedAt only after successful assignment.This prevents false positives on fetch freshness if processing fails mid-way.
79-83: Simple, clear version check aligns with maintainer guidance.Try/catch around a direct parse+compare keeps the flow straightforward per earlier preference to avoid heavy SemVer normalization helpers.
91-92: Confirm desired log level to avoid noisy manifests.Info-level logs for every excluded plugin could spam logs if many plugins require higher versions. Consider Debug level or a single summary (e.g., count + first N plugin names).
Handle MinimumAppVersion parse fail and reorder last manifest fetch date
Follows on from #3932