Reload Context #3081
Reload Context #3081
Conversation
This comment has been minimized.
This comment has been minimized.
📝 WalkthroughWalkthroughThe changes in the Changes
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: 1
🧹 Outside diff range and nitpick comments (1)
Flow.Launcher.Core/Plugin/JsonRPCPluginV2.cs (1)
Line range hint
147-161: Improve disposal pattern and error handlingWhile changing to
ValueTaskis appropriate for disposal, there are some improvements needed:
- Empty catch block silently swallows exceptions
- Disposal pattern could be more robust
Consider this improved implementation:
public virtual async ValueTask DisposeAsync() { try { if (RPC != null) { try { await RPC.InvokeAsync("close"); } catch (RemoteMethodNotFoundException e) { + Log.Exception("Plugin doesn't support close method", e); } + catch (Exception e) when (e is not ObjectDisposedException) + { + Log.Exception("Error during plugin close", e); + } } } finally { - RPC?.Dispose(); - ErrorStream?.Dispose(); + if (RPC != null) + { + RPC.Dispose(); + RPC = null; + } + if (ErrorStream != null) + { + ErrorStream.Dispose(); + ErrorStream = null; + } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
Flow.Launcher.Core/Plugin/JsonRPCPluginV2.cs(1 hunks)
🔇 Additional comments (1)
Flow.Launcher.Core/Plugin/JsonRPCPluginV2.cs (1)
Line range hint 136-161: Verify impact on other plugins and implementations
The changes to both ReloadDataAsync and DisposeAsync methods could affect other plugins and implementations.
Let's verify the impact:
✅ Verification successful
Based on the search results, I can now provide a final response:
Changes to ReloadDataAsync and DisposeAsync are safe and properly implemented
The changes to both methods are part of the standard plugin lifecycle and follow the established patterns:
-
ReloadDataAsyncis properly implemented:- Only 2 classes implement
IAsyncReloadable:JsonRPCPluginV2andProgramplugin - The reload functionality is centrally managed through
PluginManager.ReloadDataAsync() - Error handling for missing remote methods is properly handled
- Only 2 classes implement
-
DisposeAsyncfollows correct cleanup pattern:- Proper error handling for remote method calls
- Ensures cleanup of RPC and ErrorStream in finally block
- Used by derived class
ProcessStreamPluginV2which adds its own cleanup
The changes maintain compatibility with the plugin system's reload and disposal mechanisms as documented in the interfaces.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all implementations of IAsyncReloadable
echo "Searching for IAsyncReloadable implementations..."
rg "class.*:.*IAsyncReloadable" -l
# Find all reload method calls
echo "Searching for reload method calls..."
rg "ReloadDataAsync|reload" -A 5
# Find all JsonRPCPluginV2 derived classes
echo "Searching for JsonRPCPluginV2 derived classes..."
rg "class.*:.*JsonRPCPluginV2" -l
Length of output: 97681
| public virtual async Task ReloadDataAsync() | ||
| { | ||
| SetupJsonRPC(); | ||
| return Task.CompletedTask; | ||
| try | ||
| { | ||
| await RPC.InvokeAsync("reload", context); | ||
| } | ||
| catch (RemoteMethodNotFoundException e) | ||
| { | ||
| } |
There was a problem hiding this comment.
Critical: Current implementation may lead to inconsistent plugin state
The current implementation has several issues that need to be addressed:
-
The method combines both restart and reload approaches incorrectly:
- It restarts the RPC connection via
SetupJsonRPC() - It attempts to call
reloadon the new connection - It doesn't call
initializeafter restart
- It restarts the RPC connection via
-
The
contextvariable on line 141 is undefined, which will cause a compilation error. -
Empty catch block silently swallows exceptions, making it difficult to debug issues.
Consider implementing one of these solutions:
public virtual async Task ReloadDataAsync()
{
+ // Solution 1: Restart with initialize
SetupJsonRPC();
+ await RPC.InvokeAsync("initialize", Context);
}OR
public virtual async Task ReloadDataAsync()
{
- SetupJsonRPC();
try
{
await RPC.InvokeAsync("reload", Context);
}
catch (RemoteMethodNotFoundException e)
{
+ Log.Exception($"Plugin doesn't support reload method", e);
+ // Fallback to restart + initialize
+ SetupJsonRPC();
+ await RPC.InvokeAsync("initialize", Context);
}
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
@jjw24 Thank you! Let me know if you need any further assistance.
|
I think this should be consistent with the C# API, so |
|
Replying to your question in the description. Agree with Yusyuriv. It shouldn't restart plugin. It should do a reload request. I think I only saw one plugin dev using V2, so we can just let the person know. |
|
🥷 Code experts: no user matched threshold 10 See details
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. |
Yeah it's him posted the problem. Sounds good |
This comment has been minimized.
This comment has been minimized.
If I understand this correctly, won't this mean that plugin developers will have to restart flow every time they make a change? Opposed to just hitting |
Actually opposite, plugin devs can use the reload request to reload any new data without restarting flow when user presses F5. |
I meant during development. Say I make a small change of adding an icon to a certain result, with this change wouldn't I have to restart flow to have it take effect? While currently I just have to press |
|
Yeah ok that could be the case then. However V1 Python plugin didn't need to restart to use the new changes during development, is that no longer the case with V2? If so it still shouldn't be solved with the |
|
V1 doesn't have to restart. It is a new instance every single query so it is hot reloaded by nature. |
|
Oh, yes that's right. |
|
So V1 had that side effect that worked favourably during development. Hmm. |
|
I wasn't trying to get you guys to change anything, was just making sure I understood exactly what this entailed. For people who decide to use my library I can add a utility runner to re-add the functionality for development, and others can probably add something similar, or test this kind of stuff via command line or a test file. |
|
Yep, this change should make the reload API call not restart V2 plugins, just reload data like .Net plugins. Open to adding a sys plugin command to restart all plugins to help with plugin development if required. |
That would probably work better than what I mentioned in my previous comment |
@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: 1
🔭 Outside diff range comments (1)
Flow.Launcher.Core/Plugin/JsonRPCPluginV2.cs (1)
Line range hint
147-160: Add logging in the catch blockWhile the disposal of resources is handled correctly in the finally block, the empty catch block silently swallows exceptions. This can make it difficult to identify plugins that don't implement the
closemethod.Add logging to the catch block:
try { await RPC.InvokeAsync("close"); } catch (RemoteMethodNotFoundException e) { + Log.Exception("Plugin doesn't support close method", e); }
🧹 Nitpick comments (1)
Flow.Launcher.Core/Plugin/JsonRPCPluginV2.cs (1)
Line range hint
27-67: Implement consistent error handling across RPC methodsThe class shows inconsistent error handling patterns:
ExecuteResultAsync,LoadContextMenus, andQueryAsynccatch all exceptionsReloadDataAsyncandDisposeAsynconly catchRemoteMethodNotFoundExceptionThis inconsistency could make debugging difficult. Consider implementing a consistent error handling strategy across all RPC methods.
Consider extracting common RPC error handling into a helper method:
private async Task<T> InvokeRPCMethodAsync<T>(string method, object[] args = null, string errorMessage = null) { try { return await RPC.InvokeAsync<T>(method, args); } catch (RemoteMethodNotFoundException e) { Log.Exception($"Plugin doesn't support {method} method", e); throw; } catch (Exception e) { Log.Exception(errorMessage ?? $"Error invoking {method}", e); return default; } }Also applies to: 136-145
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Flow.Launcher.Core/Plugin/JsonRPCPluginV2.cs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: gitStream workflow automation
- GitHub Check: gitStream.cm
| public virtual async Task ReloadDataAsync() | ||
| { | ||
| SetupJsonRPC(); | ||
| return Task.CompletedTask; | ||
| try | ||
| { | ||
| await RPC.InvokeAsync("reload", Context); | ||
| } | ||
| catch (RemoteMethodNotFoundException e) | ||
| { | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Critical: Implementation needs to be fixed for process-based plugins
The verification revealed that ProcessStreamPluginV2 is the only direct inheritor of JsonRPCPluginV2, and it already implements proper process management. The current implementation in JsonRPCPluginV2.ReloadDataAsync() causes redundant RPC setup when called from ProcessStreamPluginV2, which could lead to race conditions.
The suggested fix remains valid and is backward compatible:
public virtual async Task ReloadDataAsync()
{
- SetupJsonRPC();
try
{
await RPC.InvokeAsync("reload", Context);
}
catch (RemoteMethodNotFoundException e)
{
+ Log.Exception("Plugin doesn't support reload method", e);
+ throw;
}
}🔗 Analysis chain
Critical: Implementation may lead to inconsistent plugin state
The current implementation has several issues:
- It incorrectly combines both restart and reload approaches by calling
SetupJsonRPC()followed by areloadrequest - It doesn't call
initializeafter restart, which may lead to an inconsistent plugin state - Empty catch block silently swallows exceptions, making debugging difficult
Based on the PR objectives and discussion consensus, implement the reload-only approach:
public virtual async Task ReloadDataAsync()
{
- SetupJsonRPC();
try
{
await RPC.InvokeAsync("reload", Context);
}
catch (RemoteMethodNotFoundException e)
{
+ Log.Exception("Plugin doesn't support reload method", e);
+ throw; // Propagate the error to inform plugin developers
}
}Let's verify if any plugins are using the old behavior:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for V2 plugins that might rely on plugin restart during reload
ast-grep --pattern 'class $_ : JsonRPCPluginV2'
Length of output: 49
Script:
#!/bin/bash
# Search for class inheritance and usages with context
rg -U "class.*:.*JsonRPCPluginV2" -A 5
rg "JsonRPCPluginV2" "*[Tt]est*.cs" -A 5
rg "ReloadDataAsync" -A 5
Length of output: 4742
There's a problem with the current implementation.
Reloadwill restart jsonrpc_v2 plugin, but not callinginitialize. This may break the plugin state that are not what plugin developer may assume.There are two solution:
initializewhenever reload.reloadrequest.This will be a breaking change regardless, but given the relatively small number of v2 plugin now it should be fine.
@Flow-Launcher/team which one would you think is better?