Skip to content

RE1-T102 Moving MCP config to normal Resgrid.Confg structure.#278

Merged
ucswift merged 1 commit intomasterfrom
develop
Feb 13, 2026
Merged

RE1-T102 Moving MCP config to normal Resgrid.Confg structure.#278
ucswift merged 1 commit intomasterfrom
develop

Conversation

@ucswift
Copy link
Member

@ucswift ucswift commented Feb 12, 2026

Summary by CodeRabbit

  • Refactor

    • Streamlined MCP server configuration management by consolidating settings into code-based configuration classes instead of application settings files.
  • Chores

    • Updated internal configuration dependencies and removed obsolete configuration references.

@request-info
Copy link

request-info bot commented Feb 12, 2026

Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details?

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 12, 2026

📝 Walkthrough

Walkthrough

This refactoring consolidates MCP server configuration from scattered sources (appsettings.json, IConfiguration injection) into centralized static config classes (McpConfig, SystemBehaviorConfig), eliminating configuration dependency injection from McpServerHost.

Changes

Cohort / File(s) Summary
New Configuration Class
Core/Resgrid.Config/McpConfig.cs
Introduces static configuration holder with three public string constants: ServerName ("Resgrid CAD System"), ServerVersion ("1.0.0"), and Transport ("stdio").
Dependency Injection Refactor
Web/Resgrid.Web.Mcp/McpServerHost.cs
Removes IConfiguration parameter from constructor and eliminates related field; updates startup logic to read ServerName and ServerVersion from McpConfig static fields instead of configuration injection.
Configuration Source Updates
Web/Resgrid.Web.Mcp/Program.cs
Replaces API base URL retrieval from configuration with SystemBehaviorConfig.ResgridApiBaseUrl; updates error messaging to reference the new configuration source.
Configuration File Cleanup
Web/Resgrid.Web.Mcp/appsettings.json
Removes McpServer configuration block and replaces with instructional comment directing configuration management to Resgrid.Config classes and environment variables.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 From scattered configs we now consolidate,
Static classes make settings first-rate!
No more injection to muddy the way,
Configuration's cleaner—hip hip hooray! 🎉

🚥 Pre-merge checks | ✅ 3 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Merge Conflict Detection ⚠️ Warning ❌ Merge conflicts detected (3 files):

⚔️ Web/Resgrid.Web.Mcp/McpServerHost.cs (content)
⚔️ Web/Resgrid.Web.Mcp/Program.cs (content)
⚔️ Web/Resgrid.Web.Mcp/appsettings.json (content)

These conflicts must be resolved before merging into master.
Resolve conflicts locally and push changes to this branch.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: migrating MCP configuration from appsettings.json to the Resgrid.Config structure with McpConfig class.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch develop
⚔️ Resolve merge conflicts (beta)
  • Auto-commit resolved conflicts to branch develop
  • Create stacked PR with resolved conflicts
  • Post resolved changes as copyable diffs in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
Web/Resgrid.Web.Mcp/Program.cs (1)

47-101: ⚠️ Potential issue | 🟡 Minor

McpConfig.Transport is defined but never consumed.

McpConfig.Transport (set to "stdio") is declared in the configuration class but is never referenced in the codebase. The transport mechanism is hardcoded as "stdio" in McpServerHost.cs (line 101) and McpServer.cs (line 43). Either use this configuration value in the server initialization, remove it if truly not needed, or add a TODO comment if intended for future implementation.

🧹 Nitpick comments (2)
Core/Resgrid.Config/McpConfig.cs (1)

7-23: Consider making fields static readonly or properties with private setters if mutation is only needed at startup.

The fields are currently publicly mutable (public static string), which means any code can change them at runtime. This is consistent with the existing SystemBehaviorConfig pattern in the codebase, but per the coding guidelines favoring immutable data, consider whether these need to be writable after initial configuration loading.

If ConfigProcessor needs to set them, a pattern like public static string ServerName { get; internal set; } = "Resgrid CAD System"; would limit mutation to the Resgrid.Config assembly.

Web/Resgrid.Web.Mcp/McpServerHost.cs (1)

42-44: Static config dependency reduces testability.

Switching from IConfiguration to McpConfig static fields creates a hidden dependency that's harder to mock in unit tests. If you want to test McpServerHost.StartAsync with different server names/versions, you'd need to mutate global state.

This is consistent with the existing Resgrid.Config pattern, so not a blocker — just noting the trade-off. If testability becomes a concern later, consider injecting an McpOptions record or similar.

@ucswift
Copy link
Member Author

ucswift commented Feb 13, 2026

Approve

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This PR is approved.

@ucswift ucswift merged commit e909879 into master Feb 13, 2026
18 of 19 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.

1 participant