Skip to content

refine indication #1114

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.

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

Draft
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

iceljc
Copy link
Collaborator

@iceljc iceljc commented Jul 28, 2025

PR Type

Enhancement


Description

  • Refine function indication system with new ChatEvent enum

  • Add indication property to ChatResponseDto for progress tracking

  • Centralize event handling with ChatHubHelper utility

  • Reorganize MessageHub namespace and improve thread safety


Diagram Walkthrough

flowchart LR
  A["ChatEvent enum"] --> B["Function execution"]
  B --> C["Indication updates"]
  C --> D["MessageHub"]
  D --> E["ChatHubHelper"]
  E --> F["SignalR clients"]
Loading

File Walkthrough

Relevant files
Enhancement
11 files
ChatResponseDto.cs
Add indication property for progress tracking                       
+4/-0     
ChatEvent.cs
Create centralized chat event constants                                   
+22/-0   
GetWeatherFn.cs
Implement progressive indication updates during execution
+26/-0   
MessageHub.cs
Add thread safety with Subject.Synchronize                             
+2/-2     
RoutingService.InvokeFunction.cs
Replace progress service with MessageHub indication           
+14/-7   
ChatHubHelper.cs
Create centralized event sending utility                                 
+38/-0   
ChatHubConversationHook.cs
Refactor to use ChatEvent constants and helper                     
+22/-148
ChatHubCrontabHook.cs
Replace hardcoded event names with ChatEvent constants     
+8/-10   
StreamingLogHook.cs
Refactor to use centralized event handling approach           
+60/-114
WelcomeHook.cs
Replace hardcoded events with ChatEvent constants               
+10/-26 
ChatHubObserver.cs
Add OnIndicationReceived event handling support                   
+80/-70 
Formatting
1 files
ConversationSenderActionModel.cs
Remove unused import statement                                                     
+0/-2     
Miscellaneous
11 files
HubObserveData.cs
Update namespace from Observables to MessageHub                   
+1/-1     
ObserveDataBase.cs
Update namespace from Observables to MessageHub                   
+1/-1     
ConversationPlugin.cs
Update imports for MessageHub namespace change                     
+2/-2     
Using.cs
Add global imports for new enums and models                           
+3/-0     
ChatCompletionProvider.cs
Update imports for MessageHub namespace change                     
+2/-2     
ChatHubPlugin.cs
Update imports for MessageHub namespace change                     
+2/-2     
Using.cs
Add ChatHubHelper to global imports                                           
+2/-1     
ChatCompletionProvider.cs
Update imports for MessageHub namespace change                     
+2/-2     
ChatCompletionProvider.cs
Update imports for MessageHub namespace change                     
+2/-2     
ChatCompletionProvider.cs
Update imports for MessageHub namespace change                     
+2/-2     
ChatCompletionProvider.cs
Update imports for MessageHub namespace change                     
+2/-2     

@iceljc iceljc marked this pull request as draft July 28, 2025 22:45
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Hardcoded Delays

The function contains hardcoded Task.Delay calls (1000ms, 1500ms, 1500ms) which will block execution and may impact performance in production. Consider making these configurable or removing them entirely.

await Task.Delay(1000);

message.Indication = "Start querying weather data";
messageHub.Push(new()
{
    EventName = ChatEvent.OnIndicationReceived,
    Data = message,
    ServiceProvider = _services
});

await Task.Delay(1500);

message.Indication = "Still working on it";
messageHub.Push(new()
{
    EventName = ChatEvent.OnIndicationReceived,
    Data = message,
    ServiceProvider = _services
});

await Task.Delay(1500);
Blocking Async Call

The code uses ConfigureAwait(false).GetAwaiter().GetResult() which can cause deadlocks in certain contexts. This synchronous blocking of async operations should be avoided.

    chatHub.Clients.Group(conversationId).SendAsync(ChatEvent.OnSenderActionGenerated, action).ConfigureAwait(false).GetAwaiter().GetResult();
}
else
{
    var user = _services.GetRequiredService<IUserIdentity>();
    chatHub.Clients.User(user.Id).SendAsync(ChatEvent.OnSenderActionGenerated, action).ConfigureAwait(false).GetAwaiter().GetResult();
Thread Safety

The MessageHub now uses Subject.Synchronize() for thread safety, but this change should be validated to ensure it doesn't introduce performance bottlenecks or unexpected behavior in high-concurrency scenarios.

private readonly ISubject<T> _observable = Subject.Synchronize(new Subject<T>());
public IObservable<T> Events => _observable;

Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix potential deadlock with async method

Using GetAwaiter().GetResult() can cause deadlocks in ASP.NET contexts and
blocks the thread unnecessarily. This should be made properly asynchronous or
use Task.Run if synchronous execution is required.

src/Plugins/BotSharp.Plugin.ChatHub/Observers/ChatHubObserver.cs [160]

-chatHub.Clients.Group(conversationId).SendAsync(ChatEvent.OnSenderActionGenerated, action).ConfigureAwait(false).GetAwaiter().GetResult();
+await chatHub.Clients.Group(conversationId).SendAsync(ChatEvent.OnSenderActionGenerated, action);
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies that using .GetAwaiter().GetResult() is a "sync-over-async" anti-pattern that can lead to deadlocks and thread pool starvation, which is a critical issue in a server application.

Medium
General
Remove hard-coded demonstration delays

Hard-coded delays in production code can negatively impact user experience and
system performance. Consider making these delays configurable or removing them
entirely if they're only for demonstration purposes.

src/Infrastructure/BotSharp.Core/Demo/Functions/GetWeatherFn.cs [23-43]

-await Task.Delay(1000);
-...
-await Task.Delay(1500);
-...
-await Task.Delay(1500);
+// Remove hard-coded delays or make them configurable
+// await Task.Delay(_settings.IndicationDelay);

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 5

__

Why: The suggestion correctly points out that hard-coded Task.Delay calls are inappropriate for production code, as they are likely placeholders for demonstration and can degrade user experience.

Low
  • More

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant