fix(dashboard): hide MCP when disabled - #577
Conversation
📝 WalkthroughWalkthroughThe dashboard runtime configuration now exposes ChangesMCP dashboard feature flag
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant DashboardRuntimeConfig
participant mcpServersPageVisible
participant fetchMcpServersPage
participant MCPServersEndpoint
DashboardRuntimeConfig-->>fetchMcpServersPage: Load runtime configuration
fetchMcpServersPage->>mcpServersPageVisible: Check MCP_ENABLED
mcpServersPageVisible-->>fetchMcpServersPage: Return visibility
alt MCP enabled
fetchMcpServersPage->>MCPServersEndpoint: Request MCP server list
else MCP disabled
fetchMcpServersPage-->>fetchMcpServersPage: Clear state and skip request
end
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/admin/dashboard/static/js/modules/mcp-servers.js`:
- Around line 294-308: Update fetchMcpServersPage so mcpServersLoading is set to
true before awaiting ensureWorkflowRuntimeConfig. Preserve the existing clearing
behavior in the disabled return path and the finally path, ensuring the loading
state remains active while runtime configuration resolves.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 408299bf-1b4a-41d5-b817-fa9b1225735f
📒 Files selected for processing (3)
internal/admin/dashboard/static/js/dashboard.jsinternal/admin/dashboard/static/js/modules/mcp-servers.jsinternal/admin/dashboard/static/js/modules/mcp-servers.test.cjs
| async fetchMcpServersPage() { | ||
| // dashboardDataFetches starts feature-backed requests in | ||
| // parallel, so wait for the shared runtime-config request | ||
| // before deciding whether the MCP admin API is available. | ||
| if (typeof this.ensureWorkflowRuntimeConfig === 'function') { | ||
| await this.ensureWorkflowRuntimeConfig(); | ||
| } | ||
| if (typeof this.mcpServersPageVisible === 'function' && !this.mcpServersPageVisible()) { | ||
| this.mcpServersAvailable = false; | ||
| this.mcpServers = []; | ||
| this.mcpServerError = ''; | ||
| this.mcpServersLoading = false; | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Set the MCP loading state before awaiting runtime configuration.
mcpServersLoading remains false while ensureWorkflowRuntimeConfig() is pending, so a direct visit can render the empty state before the feature flag resolves. Move the existing loading assignment before the await and keep clearing it in the disabled and finally paths.
Proposed fix
async fetchMcpServersPage() {
+ this.mcpServersLoading = true;
if (typeof this.ensureWorkflowRuntimeConfig === 'function') {
await this.ensureWorkflowRuntimeConfig();
}
...
- this.mcpServersLoading = true;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async fetchMcpServersPage() { | |
| // dashboardDataFetches starts feature-backed requests in | |
| // parallel, so wait for the shared runtime-config request | |
| // before deciding whether the MCP admin API is available. | |
| if (typeof this.ensureWorkflowRuntimeConfig === 'function') { | |
| await this.ensureWorkflowRuntimeConfig(); | |
| } | |
| if (typeof this.mcpServersPageVisible === 'function' && !this.mcpServersPageVisible()) { | |
| this.mcpServersAvailable = false; | |
| this.mcpServers = []; | |
| this.mcpServerError = ''; | |
| this.mcpServersLoading = false; | |
| return; | |
| } | |
| async fetchMcpServersPage() { | |
| this.mcpServersLoading = true; | |
| // dashboardDataFetches starts feature-backed requests in | |
| // parallel, so wait for the shared runtime-config request | |
| // before deciding whether the MCP admin API is available. | |
| if (typeof this.ensureWorkflowRuntimeConfig === 'function') { | |
| await this.ensureWorkflowRuntimeConfig(); | |
| } | |
| if (typeof this.mcpServersPageVisible === 'function' && !this.mcpServersPageVisible()) { | |
| this.mcpServersAvailable = false; | |
| this.mcpServers = []; | |
| this.mcpServerError = ''; | |
| this.mcpServersLoading = false; | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/admin/dashboard/static/js/modules/mcp-servers.js` around lines 294 -
308, Update fetchMcpServersPage so mcpServersLoading is set to true before
awaiting ensureWorkflowRuntimeConfig. Preserve the existing clearing behavior in
the disabled return path and the finally path, ensuring the loading state
remains active while runtime configuration resolves.
Confidence Score: 5/5The changed flow looks mergeable after making the MCP navigation gate handle unloaded or unavailable runtime config. The backend flag matches the configuration used to enable MCP. The sidebar can still show MCP while runtime config is loading or unavailable.
 T-Rex LogsWhat T-Rex did
Reviews (1): Last reviewed commit: "fix(dashboard): skip disabled MCP fetche..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Summary
Testing
Summary by CodeRabbit
New Features
Bug Fixes
Tests