Add MinimumAppVersion support for plugin.json#4299
Open
Jack251970 wants to merge 10 commits intodevfrom
Open
Conversation
Introduced MinimumAppVersion property to PluginMetadata, enabling plugins to specify required Flow Launcher version. Plugin installation now checks this requirement and prompts users if unsatisfied. Minimum app version logic moved to PluginManager and applied to manifest updates. Added localized strings for user prompts. Refactored SameOrLesserPluginVersionExists to accept PluginMetadata.
|
🥷 Code experts: jjw24 jjw24, Jack251970 have most 👩💻 activity in the files. See details
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame:
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. |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds MinimumAppVersion support to plugin metadata so plugins can declare a required Flow Launcher version, with install-time prompting and manifest filtering when requirements aren’t met.
Changes:
- Added
MinimumAppVersiontoPluginMetadata(read fromplugin.json). - Added a minimum-version check during plugin installation with a user confirmation prompt.
- Reused the same minimum-version check to filter plugins during manifest update, plus new localized strings.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| Flow.Launcher/Languages/en.xaml | Adds localized title/message for the minimum app version warning prompt. |
| Flow.Launcher.Plugin/PluginMetadata.cs | Introduces MinimumAppVersion on plugin metadata to support compatibility constraints. |
| Flow.Launcher.Core/Plugin/PluginManager.cs | Implements compatibility check and prompts during install; exposes shared version-satisfaction helper. |
| Flow.Launcher.Core/ExternalPlugins/PluginsManifest.cs | Filters manifest plugins via the shared compatibility check. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughDelegates plugin minimum-app-version checks to PluginManager.IsMinimumAppVersionSatisfied, adds MinimumAppVersion to PluginMetadata, updates PluginManager install/version checks to use PluginMetadata (with user prompt when current Flow version is below required), and adds localized strings for related install/error messages. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as Installer/UI
participant PluginManager
participant Metadata as PluginMetadata
participant Dialog as ConfirmationDialog
User->>UI: Start plugin install
UI->>PluginManager: Begin install (package path)
PluginManager->>Metadata: Load and deserialize plugin.json (PluginMetadata with MinimumAppVersion)
Metadata-->>PluginManager: PluginMetadata
PluginManager->>PluginManager: SameOrLesserPluginVersionExists(metadata)
PluginManager->>PluginManager: IsMinimumAppVersionSatisfied(metadata.Name, metadata.MinimumAppVersion)
alt MinimumAppVersion not satisfied
PluginManager->>Dialog: Show unsatisfied-version prompt (title/message)
Dialog->>User: Ask to continue despite version mismatch
User-->>Dialog: Confirm or Cancel
alt Confirm
PluginManager->>UI: Proceed with install
PluginManager-->>UI: Install completed
else Cancel
PluginManager-->>UI: Install aborted
end
else Satisfied
PluginManager->>UI: Proceed with install
PluginManager-->>UI: Install completed
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
Flow.Launcher.Core/Plugin/PluginManager.cs (1)
1080-1086: Use context-neutral log text in shared version-check helper.The message “Plugin excluded from manifest” is inaccurate when this method is called from install flow. Use neutral wording to avoid misleading diagnostics.
♻️ Proposed tweak
- PublicApi.Instance.LogException(ClassName, $"Failed to parse the minimum app version {minimumAppVersion} for plugin {pluginName}. " - + "Plugin excluded from manifest", e); + PublicApi.Instance.LogException(ClassName, $"Failed to parse minimum app version {minimumAppVersion} for plugin {pluginName}.", e); ... - PublicApi.Instance.LogInfo(ClassName, $"Plugin {pluginName} requires minimum Flow Launcher version {minimumAppVersion}, " - + $"but current version is {Constant.Version}. Plugin excluded from manifest."); + PublicApi.Instance.LogInfo(ClassName, $"Plugin {pluginName} requires minimum Flow Launcher version {minimumAppVersion}, " + + $"but current version is {Constant.Version}.");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Flow.Launcher.Core/Plugin/PluginManager.cs` around lines 1080 - 1086, The log messages emitted by PublicApi.Instance.LogException and PublicApi.Instance.LogInfo in PluginManager (referencing ClassName, minimumAppVersion, pluginName, Constant.Version) use the phrase "Plugin excluded from manifest", which is misleading in non-manifest contexts (e.g., install flow); update both messages to context-neutral wording such as "Plugin skipped" or "Plugin not processed" (or similar) so the logs correctly reflect that the plugin was not processed without implying manifest-specific behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Flow.Launcher.Core/Plugin/PluginManager.cs`:
- Around line 1071-1076: Move the parsing of Constant.Version into the guarded
path or use TryParse for both values: replace the direct
Version.Parse(Constant.Version) usage (which populates appVersion) with a safe
Version.TryParse for Constant.Version and Version.TryParse for
minimumAppVersion, validate both parses before comparing, and return false (or
handle accordingly) when either parse fails; ensure the comparison logic that
previously used appVersion and minimumAppVersion still occurs only after
successful TryParse results so no parsing exceptions escape the try/catch in the
PluginManager method.
- Around line 899-914: The deserialization of plugin metadata using
JsonSerializer.Deserialize<PluginMetadata>(File.ReadAllText(metadataJsonFilePath))
is unguarded and may throw JsonException or return null; wrap that call in a
try-catch and validate newMetadata for null before dereferencing it in
SameOrLesserPluginVersionExists(newMetadata) and
IsMinimumAppVersionSatisfied(newMetadata.Name, ...). On exception or null, log
or surface a user-friendly error (e.g., via PublicApi.Instance.ShowMsgError with
Localize.failedToInstallPluginTitle(plugin.Name) and an appropriate message) and
return false so the install path is aborted safely.
---
Nitpick comments:
In `@Flow.Launcher.Core/Plugin/PluginManager.cs`:
- Around line 1080-1086: The log messages emitted by
PublicApi.Instance.LogException and PublicApi.Instance.LogInfo in PluginManager
(referencing ClassName, minimumAppVersion, pluginName, Constant.Version) use the
phrase "Plugin excluded from manifest", which is misleading in non-manifest
contexts (e.g., install flow); update both messages to context-neutral wording
such as "Plugin skipped" or "Plugin not processed" (or similar) so the logs
correctly reflect that the plugin was not processed without implying
manifest-specific behavior.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Flow.Launcher.Core/ExternalPlugins/PluginsManifest.csFlow.Launcher.Core/Plugin/PluginManager.csFlow.Launcher.Plugin/PluginMetadata.csFlow.Launcher/Languages/en.xaml
Previously, installing a plugin with an invalid or corrupted plugin.json would cause an unhandled exception. Now, deserialization errors are caught, a user-friendly error message is shown, and the exception is logged. Added a new localized error message for this scenario in en.xaml.
prlabeler
bot
added
bug
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Flow.Launcher.Core/Plugin/PluginManager.cs">
<violation number="1" location="Flow.Launcher.Core/Plugin/PluginManager.cs:902">
P2: `JsonSerializer.Deserialize` can return null (e.g., `plugin.json` contains `null`), which will bypass the catch and cause a null reference when `newMetadata` is used. Add an explicit null check and handle it as an invalid/corrupted manifest.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
coderabbitai
bot
removed
bug
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Flow.Launcher.Core/Plugin/PluginManager.cs`:
- Around line 899-913: After calling
JsonSerializer.Deserialize<PluginMetadata>(File.ReadAllText(metadataJsonFilePath))
assign to newMetadata and immediately check if newMetadata is null; if it is,
show the same user error and log the exception context (use
PublicApi.Instance.ShowMsgError(Localize.failedToInstallPluginTitle(plugin.Name),
Localize.pluginJsonInvalidOrCorrupted()) and
PublicApi.Instance.LogException(ClassName, $"Failed to deserialize plugin
metadata for plugin {plugin.Name} from file {metadataJsonFilePath} -
deserialized null", null) or similar) and return false before calling
SameOrLesserPluginVersionExists(newMetadata) so downstream access like
newMetadata.Version cannot throw.
Refactored the plugin minimum app version check to use Version.TryParse instead of try-catch with Version.Parse, preventing exceptions on invalid input. Improved error handling by logging parse failures as errors and version mismatches as info, making the logic clearer and more robust.
prlabeler
bot
added
bug
Throw a JsonException if deserializing plugin metadata from JSON returns null. This prevents null metadata from being used and improves error handling for invalid or corrupted plugin.json files.
Improved the message shown when a plugin requires a newer Flow Launcher version by adding line breaks for clarity and updating the title wording. Adjusted code to pass Environment.NewLine and modified resource strings in en.xaml to support the new format.
Jack251970
changed the title
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Flow.Launcher.Core/Plugin/PluginManager.cs (1)
1079-1101: Consider making log messages context-agnostic.The log messages at lines 1090 and 1098 state "Plugin excluded from manifest", but this method is also called during plugin installation (line 921) where that message is contextually misleading. Consider using a more generic message like "Plugin minimum version requirement not satisfied".
This is a minor clarity issue and can be addressed in a follow-up if desired.
♻️ Proposed fix for log message clarity
if (!Version.TryParse(minimumAppVersion, out var minimumVersion)) { PublicApi.Instance.LogError(ClassName, $"Failed to parse the minimum app version {minimumAppVersion} for plugin {pluginName}. " - + "Plugin excluded from manifest"); + + "Minimum version requirement check failed"); return false; // If the minimum app version specified in plugin.json is invalid, we assume it is not satisfied } if (appVersion >= minimumVersion) return true; PublicApi.Instance.LogInfo(ClassName, $"Plugin {pluginName} requires minimum Flow Launcher version {minimumAppVersion}, " - + $"but current version is {Constant.Version}. Plugin excluded from manifest."); + + $"but current version is {Constant.Version}. Minimum version requirement not satisfied."); return false;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Flow.Launcher.Core/Plugin/PluginManager.cs` around lines 1079 - 1101, Update the log messages in IsMinimumAppVersionSatisfied to be context-agnostic: when Version.TryParse fails, change the PublicApi.Instance.LogError call that currently includes "Plugin excluded from manifest" to a generic message like "Plugin minimum version requirement invalid" (referencing pluginName and minimumAppVersion); similarly update the PublicApi.Instance.LogInfo call that says "Plugin excluded from manifest" to a neutral message such as "Plugin minimum version requirement not satisfied" (include pluginName, minimumAppVersion and Constant.Version). Ensure only the message strings are changed in the IsMinimumAppVersionSatisfied method so callers (installation or manifest loading) receive non-misleading logs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Flow.Launcher.Core/Plugin/PluginManager.cs`:
- Around line 1079-1101: Update the log messages in IsMinimumAppVersionSatisfied
to be context-agnostic: when Version.TryParse fails, change the
PublicApi.Instance.LogError call that currently includes "Plugin excluded from
manifest" to a generic message like "Plugin minimum version requirement invalid"
(referencing pluginName and minimumAppVersion); similarly update the
PublicApi.Instance.LogInfo call that says "Plugin excluded from manifest" to a
neutral message such as "Plugin minimum version requirement not satisfied"
(include pluginName, minimumAppVersion and Constant.Version). Ensure only the
message strings are changed in the IsMinimumAppVersionSatisfied method so
callers (installation or manifest loading) receive non-misleading logs.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Flow.Launcher.Core/Plugin/PluginManager.csFlow.Launcher/Languages/en.xaml
🚧 Files skipped from review as they are similar to previous changes (1)
- Flow.Launcher/Languages/en.xaml
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
prlabeler
bot
added
bug
Logging when a plugin's minimum Flow Launcher version is not met has been removed. The method now returns false without logging any informational message.
coderabbitai
bot
removed
bug
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Flow.Launcher/Languages/en.xaml (1)
235-235: Clarify minimum-version wording in dialog title.Line 235 reads like an exact-version requirement. Since this flow checks a minimum version, “or later” avoids confusion.
💬 Suggested wording tweak
- <system:String x:Key="pluginMinimumAppVersionUnsatisfiedTitle">{0} requires Flow version {1} to run</system:String> + <system:String x:Key="pluginMinimumAppVersionUnsatisfiedTitle">{0} requires Flow version {1} or later to run</system:String>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Flow.Launcher/Languages/en.xaml` at line 235, Update the resource string keyed by pluginMinimumAppVersionUnsatisfiedTitle so the title expresses a minimum version requirement (e.g. change "{0} requires Flow version {1} to run" to "{0} requires Flow version {1} or later to run"); locate the system:String with x:Key="pluginMinimumAppVersionUnsatisfiedTitle" and edit its value accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Flow.Launcher.Core/Plugin/PluginManager.cs`:
- Around line 921-931: The early return when PublicApi.Instance.ShowMsgBox
returns MessageBoxResult.No skips the later cleanup of the extracted temp plugin
folder; to fix, ensure the temp extraction cleanup is always executed before
returning false by moving the minimum-version user-prompt logic inside the
existing try/finally (or call the same cleanup routine) so that the cleanup code
that deletes the extracted temp folder runs even when the user cancels. Locate
the check using IsMinimumAppVersionSatisfied and PublicApi.Instance.ShowMsgBox
(and the newMetadata variable) and either call the same temp-folder cleanup
helper immediately before returning false or refactor the surrounding method to
put the prompt inside the try/finally that invokes the cleanup block.
---
Nitpick comments:
In `@Flow.Launcher/Languages/en.xaml`:
- Line 235: Update the resource string keyed by
pluginMinimumAppVersionUnsatisfiedTitle so the title expresses a minimum version
requirement (e.g. change "{0} requires Flow version {1} to run" to "{0} requires
Flow version {1} or later to run"); locate the system:String with
x:Key="pluginMinimumAppVersionUnsatisfiedTitle" and edit its value accordingly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Introduced MinimumAppVersion property to PluginMetadata, enabling plugins to specify required Flow Launcher version. Plugin installation now checks this requirement and prompts users if unsatisfied. Developer can set
MinimumAppVersionin theplugin.jsonof their plugin zip package.Summary by cubic
Adds MinimumAppVersion support in plugin.json with centralized checks in PluginManager to block installs on unsupported Flow versions and prompt when requirements aren’t met. Solves the issue of plugins installing below the required Flow version and crashing on invalid or corrupted plugin.json; this PR closes that issue.
New Features
Bug Fixes
Written for commit cce80ac. Summary will update on new commits.