Skip to content

feat: Add service-aware AttachMenu overload to IBotSharpPlugin so plu…#1380

Merged
iceljc merged 1 commit into
SciSharp:masterfrom
ywang1110:feature/attachmenu-serviceprovider-overload
Jul 16, 2026
Merged

feat: Add service-aware AttachMenu overload to IBotSharpPlugin so plu…#1380
iceljc merged 1 commit into
SciSharp:masterfrom
ywang1110:feature/attachmenu-serviceprovider-overload

Conversation

@ywang1110

Copy link
Copy Markdown
Contributor

feat: Add service-aware AttachMenu overload to IBotSharpPlugin so plugins can build per-user menu entries

@qodo-code-review

Copy link
Copy Markdown
Contributor

PR Summary by Qodo

Add request-scoped IServiceProvider AttachMenu overload for plugins

✨ Enhancement 🕐 10-20 Minutes

Grey Divider

AI Description

• Add an async, service-aware AttachMenu overload to IBotSharpPlugin.
• Invoke menu attachment with the request-scoped IServiceProvider in PluginController.
• Enable plugins to generate per-user menu entries using scoped services.
Diagram

graph TD
  A["PluginController"] --> B["IBotSharpPlugin"] --> C["AttachMenu(menu, services)"]
  C --> D[("Request-scoped IServiceProvider")]
  C --> E["AttachMenu(menu) default"]

  subgraph Legend
    direction LR
    _ctrl["Controller"] ~~~ _api["Interface/API"] ~~~ _svc[("DI/Service Provider")] ~~~ _fn["Method"]
  end
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Pass an explicit menu context (e.g., user/tenant info) instead of IServiceProvider
  • ➕ Avoids service-locator pattern; clearer contract of what plugins may depend on
  • ➕ Easier to test (simple DTO instead of DI container)
  • ➕ Reduces risk of plugins resolving unintended services
  • ➖ Requires designing and evolving a context model over time
  • ➖ May still need a way to access additional services for complex plugins
2. Register menu contributors via DI (IMenuContributor) resolved per request
  • ➕ Keeps request-scope resolution inside host; plugins expose concrete contributors
  • ➕ Encourages composable, testable units; avoids passing IServiceProvider around
  • ➖ More structural change (new abstraction, discovery/registration pattern)
  • ➖ Potentially larger migration for existing plugins

Recommendation: The chosen approach is pragmatic and backward-compatible: adding an async overload with a default implementation preserves existing plugins while enabling per-request/per-user menu logic. If menu generation becomes more complex or security-sensitive, consider moving toward an explicit context object or DI-resolved contributors to avoid broad IServiceProvider exposure.

Files changed (2) +8 / -1

Enhancement (2) +8 / -1
IBotSharpPlugin.csAdd async AttachMenu overload accepting IServiceProvider +7/-0

Add async AttachMenu overload accepting IServiceProvider

• Introduces a service-aware, async AttachMenu overload that receives the request-scoped IServiceProvider. Provides a default implementation that delegates to the existing synchronous AttachMenu(List<PluginMenuDef>) overload for backward compatibility.

src/Infrastructure/BotSharp.Abstraction/Plugins/IBotSharpPlugin.cs

PluginController.csCall service-aware AttachMenu from GetPluginMenu +1/-1

Call service-aware AttachMenu from GetPluginMenu

• Updates menu aggregation to await the new AttachMenu(menu, services) overload, passing the request-scoped service provider. Enables plugins to resolve scoped dependencies when constructing menu entries.

src/Infrastructure/BotSharp.OpenAPI/Controllers/Setting/PluginController.cs

ⓘ You are approaching your monthly quota for Qodo. Upgrade your plan

@qodo-code-review

Copy link
Copy Markdown
Contributor

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Grey Divider

ⓘ You are approaching your monthly quota for Qodo. Upgrade your plan

Qodo Logo

@iceljc
iceljc merged commit 960b623 into SciSharp:master Jul 16, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants