Support Assignment of One Action Keyword with Multiple Plugins#4295
Open
Jack251970 wants to merge 15 commits intodevfrom
Open
Support Assignment of One Action Keyword with Multiple Plugins#4295Jack251970 wants to merge 15 commits intodevfrom
Jack251970 wants to merge 15 commits intodevfrom
Conversation
Mark ActionKeywordRegistered as obsolete and always return false, reflecting support for multiple plugins per action keyword. Update IPublicAPI docs to clarify ActionKeywordAssigned is for legacy compatibility.
Expanded using directives for .NET collections and diagnostics. Added ActionKeywordAssigned to PublicAPIInstance, using obsolete PluginManager.ActionKeywordRegistered with warning suppression.
Refactor non-global plugin storage to allow multiple plugins to share the same action keyword by using ConcurrentDictionary<string, List<PluginPair>>. Update all relevant methods to handle lists of plugins, ensure thread safety, and adjust the QueryBuilder logic accordingly. This change improves extensibility and flexibility in plugin management.
Jack251970
changed the title
|
🥷 Code experts: jjw24 Jack251970, jjw24 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. |
Jack251970
changed the title
Jack251970
changed the title
![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 4 files
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:391">
P2: `plugins` is a mutable List<PluginPair> that is modified under locks elsewhere, but here it’s enumerated without synchronization. Concurrent updates can throw during query execution. Take a snapshot of the list under the same lock before filtering.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates Flow Launcher’s action keyword routing to support multiple plugins sharing the same non-global action keyword, and keeps the old “keyword already assigned” check only for backward compatibility.
Changes:
- Switch non-global action keyword storage from a single
PluginPairtoList<PluginPair>per keyword. - Update query parsing/selection logic to treat an action keyword as valid if any plugin for that keyword is enabled.
- Mark the old registration check as obsolete and suppress obsolete warnings where it’s used via
IPublicAPI.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| Flow.Launcher/PublicAPIInstance.cs | Suppresses obsolete warning when calling the legacy action keyword assignment check. |
| Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs | Adds remarks clarifying ActionKeywordAssigned is now for compatibility, but needs doc cleanup. |
| Flow.Launcher.Core/Plugin/QueryBuilder.cs | Updates query-building to use keyword→multiple-plugins mapping and checks enabled plugins via LINQ. |
| Flow.Launcher.Core/Plugin/PluginManager.cs | Implements keyword→multiple-plugins mapping and updates keyword add/remove/uninstall logic accordingly. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughSwitched non-global plugin storage to allow multiple plugins per action keyword (values now List), updated registration/lookup/removal/uninstall paths, adapted QueryBuilder and tests to the new shape, and marked ActionKeywordRegistered/ActionKeywordAssigned obsolete for backward compatibility. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant PM as PluginManager
participant QB as QueryBuilder
participant Plugin as Plugin(s)
rect rgba(200,200,255,0.5)
User->>PM: Register plugin with action keyword
PM->>PM: Add PluginPair to _nonGlobalPlugins[key] (lock per-key)
end
rect rgba(200,255,200,0.5)
User->>QB: Issue query with action keyword
QB->>PM: GetNonGlobalPlugins()
PM-->>QB: Dictionary<string, List<PluginPair>>
QB->>QB: CheckPlugin(list) -> select enabled plugin(s)
QB->>Plugin: Invoke selected plugin(s)
Plugin-->>User: Return results
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
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: 4
🧹 Nitpick comments (1)
Flow.Launcher.Core/Plugin/PluginManager.cs (1)
1067-1073: Consider pruning empty keyword buckets during uninstall cleanup.After removals, empty lists remain in
_nonGlobalPlugins. Removing empty keys would keep lookups cleaner and avoid stale buckets over time.🤖 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 1067 - 1073, After removing plugin entries from each bucket in entriesToUpdate, prune any buckets that become empty by removing their key from _nonGlobalPlugins; inside the loop that iterates entriesToUpdate (where you currently call entry.Value.RemoveAll(p => p.Metadata.ID == plugin.ID)), check if entry.Value.Count == 0 and if so remove the corresponding key from _nonGlobalPlugins (use the appropriate synchronization used for _nonGlobalPlugins—e.g., lock(_nonGlobalPlugins) or its concurrent remove method—to avoid race conditions). Ensure you reference the same bucket via entry.Key when removing the key.
🤖 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 757-769: The add/update logic for plugin keywords in PluginManager
(_nonGlobalPlugins) prevents duplicate plugin entries but still appends
duplicate keywords into plugin metadata, causing drift; update the AddOrUpdate
lambdas (the block adding to _nonGlobalPlugins and the similar block around
lines 803-809) to deduplicate metadata when adding (e.g., check existing
metadata before appending or use a HashSet-style temporary collection) and to
clean up metadata when removing a plugin (remove only the plugin's keyword and
avoid leaving stale duplicates), preserving the existing lock on the list to
remain thread-safe and returning the updated collection.
- Around line 383-395: When fetching keyword matches from _nonGlobalPlugins,
filter out disabled plugins immediately so disabled siblings are never returned;
modify the branch that handles the TryGetValue result to set plugins =
plugins.Where(p => !PluginModified(p.Metadata.ID)) before applying the
dialogJump filter, then if dialogJump further restrict to p.Plugin is
IAsyncDialogJump and finally return the filtered collection (same pattern
applies to the GetGlobalPlugins() early-return branches—ensure they also include
!PluginModified(p.Metadata.ID) when building their result).
- Around line 589-592: GetNonGlobalPlugins currently returns a shallow copy of
_nonGlobalPlugins exposing the original mutable List<PluginPair> instances,
which can be concurrently mutated and cause InvalidOperationException; fix it by
taking a lock on the backing collection (_nonGlobalPlugins) while enumerating
and construct a new Dictionary where each value is a fresh snapshot list (e.g.,
new List<PluginPair>(existingList) or existingList.ToList()) so callers receive
independent lists; update the GetNonGlobalPlugins method to lock
_nonGlobalPlugins during the copy and return those cloned lists (referencing
GetNonGlobalPlugins, _nonGlobalPlugins, and PluginPair to locate the change).
In `@Flow.Launcher.Plugin/Interfaces/IPublicAPI.cs`:
- Around line 296-299: The XML <remarks> text in IPublicAPI.cs is unclear and
grammatically incorrect; replace it with a concise, polished legacy-only
statement (update the <remarks> block for the affected method in the IPublicAPI
interface) such as: "Flow now allows a single action keyword to be associated
with multiple plugins; this method is retained only for legacy compatibility and
may be removed in a future release." Ensure the new wording explicitly states
legacy-only intent and is grammatically correct.
---
Nitpick comments:
In `@Flow.Launcher.Core/Plugin/PluginManager.cs`:
- Around line 1067-1073: After removing plugin entries from each bucket in
entriesToUpdate, prune any buckets that become empty by removing their key from
_nonGlobalPlugins; inside the loop that iterates entriesToUpdate (where you
currently call entry.Value.RemoveAll(p => p.Metadata.ID == plugin.ID)), check if
entry.Value.Count == 0 and if so remove the corresponding key from
_nonGlobalPlugins (use the appropriate synchronization used for
_nonGlobalPlugins—e.g., lock(_nonGlobalPlugins) or its concurrent remove
method—to avoid race conditions). Ensure you reference the same bucket via
entry.Key when removing the key.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Flow.Launcher.Core/Plugin/PluginManager.csFlow.Launcher.Core/Plugin/QueryBuilder.csFlow.Launcher.Plugin/Interfaces/IPublicAPI.csFlow.Launcher/PublicAPIInstance.cs
GetNonGlobalPlugins now returns a new dictionary with copied lists, preventing external modification of the internal _nonGlobalPlugins collection. This change improves encapsulation and safeguards plugin manager state integrity.
Replaced direct _nonGlobalPlugins field access with the GetNonGlobalPlugins() method to improve encapsulation and ensure up-to-date plugin data is used when retrieving non-global plugins by action keyword. No other logic was changed.
Wrap kvp.Value access in a lock when copying to nonGlobalPlugins to prevent race conditions and ensure thread safety during concurrent access.
Prevent duplicate action keywords by checking for existence before adding. Remove all instances of an old action keyword instead of just the first. Ensures action keyword lists remain unique and consistent.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Previously, the code only excluded modified plugins from the valid plugin list. This update adds an additional check to also exclude plugins marked as disabled in their metadata, ensuring that disabled plugins are not considered valid or processed further.
After removing plugins, also remove dictionary entries if their associated lists become empty. This prevents unused empty lists from accumulating and keeps the plugin manager's state clean.
Refactored QueryBuilderTest.cs to use Dictionary<string, List<PluginPair>> for nonGlobalPlugins, updating test cases to use lists of PluginPair. Also replaced empty dictionary instantiation with shorthand [] where appropriate.
Extract plugin enabled check into a new CheckPlugin method, adding a lock for thread safety. Update the if statement to use this method instead of inline logic.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Flow.Launcher.Test/QueryBuilderTest.cs (1)
34-49:⚠️ Potential issue | 🟡 MinorTest logic inconsistency: plugin is not actually disabled.
The test name
ExclusivePluginQueryIgnoreDisabledTestsuggests it should verify that disabled plugins are ignored, but thePluginMetadatacreated on line 39 doesn't setDisabled = true. The default value ofDisabledis typicallyfalse, so the plugin is enabled and its action keyword should be recognized.The test assertion on line 47 expects
ActionKeywordto NOT equal">", which would only be true if the plugin were disabled. Either the test data or the test assertions need correction.💚 Proposed fix to actually disable the plugin
var nonGlobalPlugins = new Dictionary<string, List<PluginPair>> { - { ">", new List<PluginPair>(){ new() { Metadata = new PluginMetadata { ActionKeywords = [">"] } } } } + { ">", new List<PluginPair>(){ new() { Metadata = new PluginMetadata { ActionKeywords = [">"], Disabled = true } } } } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Flow.Launcher.Test/QueryBuilderTest.cs` around lines 34 - 49, The test ExclusivePluginQueryIgnoreDisabledTest is currently creating a PluginMetadata without marking it disabled, so the plugin is treated as enabled; update the test data to set PluginMetadata.Disabled = true for the PluginPair (or otherwise supply a disabled plugin) so QueryBuilder.Build sees the plugin as disabled and the assertion ClassicAssert.AreNotEqual(">", q.ActionKeyword) becomes valid; locate the PluginPair instantiation in the test and add Disabled = true to the PluginMetadata before calling QueryBuilder.Build (verify QueryBuilder.Build and Query properties like ActionKeyword, Search, TrimmedQuery, SearchTerms, SecondToEndSearch behave as asserted).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@Flow.Launcher.Test/QueryBuilderTest.cs`:
- Around line 34-49: The test ExclusivePluginQueryIgnoreDisabledTest is
currently creating a PluginMetadata without marking it disabled, so the plugin
is treated as enabled; update the test data to set PluginMetadata.Disabled =
true for the PluginPair (or otherwise supply a disabled plugin) so
QueryBuilder.Build sees the plugin as disabled and the assertion
ClassicAssert.AreNotEqual(">", q.ActionKeyword) becomes valid; locate the
PluginPair instantiation in the test and add Disabled = true to the
PluginMetadata before calling QueryBuilder.Build (verify QueryBuilder.Build and
Query properties like ActionKeyword, Search, TrimmedQuery, SearchTerms,
SecondToEndSearch behave as asserted).
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/QueryBuilder.cs`:
- Around line 64-70: Remove the redundant lock in CheckPlugin: the method
currently locks on pluginPairs but callers use lists returned from
PluginManager.GetNonGlobalPlugins(), which already makes locked copies, so
thread safety is ensured at the source; update CheckPlugin (used by
QueryBuilder.Build) to simply return pluginPairs.Any(plugin =>
!plugin.Metadata.Disabled) without the lock, leaving synchronization
responsibility to PluginManager.GetNonGlobalPlugins().
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 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>
ExclusivePluginQueryIgnoreDisabledTest now sets the ">" plugin's Disabled property to true in PluginMetadata. This verifies that QueryBuilder correctly ignores disabled exclusive plugins.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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.
CHANGES
Allows multiple plugins to share the same action keyword, removing the one-to-one limitation and improving plugin flexibility.
Updated
IPublicAPIdocumentation and implementation to clarify thatActionKeywordAssignedis now only for compatibility with old plugins, and suppressed obsolete warnings in its usage.TEST
Hello World Python&Plugin Indicatorhave the same action keyword?.