Skip to content
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.

Sign up for GitHub

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

Reload Context #3081

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from
Open

Reload Context #3081

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5a80adc
Send a reload request with ctx when reloading
taooceros Nov 18, 2024
b5c264f
Merge branch 'dev' into taooceros-patch-1
taooceros Dec 2, 2024
2a1d502
fix build error
taooceros Jan 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions Flow.Launcher.Core/Plugin/JsonRPCPluginV2.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,10 +133,16 @@ private void SetupJsonRPC()
RPC.StartListening();
}

public virtual Task ReloadDataAsync()
public virtual async Task ReloadDataAsync()
{
SetupJsonRPC();
return Task.CompletedTask;
try
{
await RPC.InvokeAsync("reload", context);
}
catch (RemoteMethodNotFoundException e)
{
}
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Critical: Current implementation may lead to inconsistent plugin state

The current implementation has several issues that need to be addressed:

  1. The method combines both restart and reload approaches incorrectly:

    • It restarts the RPC connection via SetupJsonRPC()
    • It attempts to call reload on the new connection
    • It doesn't call initialize after restart

  2. The context variable on line 141 is undefined, which will cause a compilation error.

  3. 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work @coderabbitai

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jjw24 Thank you! Let me know if you need any further assistance.

}

public virtual async ValueTask DisposeAsync()
Expand Down
Loading