Skip to content

Conversation

@cmppoon
Copy link
Contributor

@cmppoon cmppoon commented Aug 1, 2025

Describe Your Changes

This PR fixes some bugs that I found when editing mcp servers json:

  • When editing an individual MCP server's JSON, setting "active" to false doesn't work. It automatically toggles back to true.
  • When editing the JSON for all MCP servers, "active" can be set to false, but the connection indicator remains blue and does not reflect the change.
  • Also fixed some minor issues:
    • syncServers and syncServersAndRestart return type should be Promise.
    • Removed unnecessary await when calling toggleServer as it does not return a promise.

Please let me know if this is the expected behavior or if any changes are required from my side. Thanks!

Fixes Issues

  • Closes #
  • Closes #

Self Checklist

  • Added relevant comments, esp in complex areas
  • Updated docs (for bug fixes / features)
  • Created issues for follow-up changes or refactoring needed

Important

Fixes MCP server JSON editing issues and updates function return types and code efficiency.

  • Behavior:
    • Fixes issue where setting "active" to false in individual MCP server JSON did not persist.
    • Fixes issue where "active" status in all MCP servers JSON did not update connection indicator.
  • Functions:
    • Changes syncServers and syncServersAndRestart return types to Promise<void> in useMCPServers.ts.
    • Removes unnecessary await in toggleServer calls in mcp-servers.tsx.
  • Misc:
    • Minor error handling improvements in mcp-servers.tsx.

This description was created by Ellipsis for 9cb8445. You can customize this summary. It will automatically update as commits are pushed.

Copy link
Contributor

@ellipsis-dev ellipsis-dev 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

Changes requested ❌

Reviewed everything up to 9cb8445 in 2 minutes and 0 seconds. Click for details.
  • Reviewed 73 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. web-app/src/hooks/useMCPServers.ts:27
  • Draft comment:
    Good update: syncServers and syncServersAndRestart now return Promise to reflect their async operations.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None
2. web-app/src/routes/settings/mcp-servers.tsx:156
  • Draft comment:
    Using the active flag from the JSON config (i.e. (data as MCPServerConfig).active || false) correctly fixes the bug where the server was always reactivated. Good improvement.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None

Workflow ID: wflow_eabin9j7WfYpSyeX

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

@louis-jan louis-jan self-requested a review August 3, 2025 14:17
@louis-jan
Copy link
Contributor

Thanks for the PR! I’ll test it out soon

Copy link
Contributor

@louis-jan louis-jan left a comment

Choose a reason for hiding this comment

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

LGTM

@louis-jan louis-jan force-pushed the fix-mcp-servers-edit-json branch from 5569a2c to da0cf10 Compare August 5, 2025 01:09
@louis-jan louis-jan merged commit 4800402 into janhq:dev Aug 5, 2025
15 of 18 checks passed
@github-project-automation github-project-automation bot moved this to QA in Jan Aug 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: QA

Development

Successfully merging this pull request may close these issues.

2 participants