-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Always allow MCP for web #6462
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
Always allow MCP for web #6462
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed everything up to dddad28 in 1 minute and 47 seconds. Click for details.
- Reviewed
308lines of code in6files - Skipped
0files when reviewing. - Skipped posting
6draft 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/containers/SettingsMenu.tsx:107
- Draft comment:
Changed MCP and Extensions menu items to use MCP_SERVERS_SETTINGS and EXTENSIONS_SETTINGS flags. Update docs to explain the new behavior. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
2. web-app/src/hooks/useToolApproval.ts:37
- Draft comment:
Setting allowAllMCPPermissions using MCP_AUTO_APPROVE_TOOLS enables auto-approval on web. Verify if this is the intended UX for non-tauri platforms. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
3. web-app/src/lib/platform/const.ts:44
- Draft comment:
New feature flags (MCP_AUTO_APPROVE_TOOLS, MCP_SERVERS_SETTINGS, EXTENSIONS_SETTINGS) differentiate between web and Tauri. Consider adding inline comments to clarify that MCP servers and extension settings are intentionally hidden on web. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
4. web-app/src/lib/platform/types.ts:40
- Draft comment:
Replacing EXTENSION_MANAGEMENT and MCP_SERVERS with new enum values improves clarity. Confirm that all references are updated and the documentation reflects these changes. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
5. web-app/src/routes/settings/extensions.tsx:21
- Draft comment:
Updated PlatformGuard in the extensions route to use the EXTENSIONS_SETTINGS flag to hide extension management on web as intended. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
6. web-app/src/routes/settings/mcp-servers.tsx:93
- Draft comment:
Updated PlatformGuard to use MCP_SERVERS_SETTINGS and removed the separate web version; ensure these changes do not negatively impact Tauri behavior. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
Workflow ID: wflow_EcvWZfQoaLO6mc5L
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed 4e633f0 in 2 minutes and 12 seconds. Click for details.
- Reviewed
26lines of code in1files - Skipped
0files when reviewing. - Skipped posting
6draft 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/test/setup.ts:13
- Draft comment:
Removed extensionManagement flag. Confirm no tests or code still depend on it. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to confirm that no tests or code still depend on the removed flag. This is against the rule of not asking the author to confirm or double-check things. The comment does not provide a specific suggestion or point out a specific issue.
2. web-app/src/test/setup.ts:15
- Draft comment:
Removed mcpServers flag. Ensure replacement with new MCP settings (mcpServersSettings) is complete. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the author to ensure that the replacement of a flag is complete. This is similar to asking the author to double-check their work, which violates the rules. The comment does not provide a specific suggestion or ask for a specific test to be written.
3. web-app/src/test/setup.ts:21
- Draft comment:
Changed webAutoModelSelection from true to false. Verify that disabling auto model selection is intentional. - Reason this comment was not posted:
Comment looked like it was already resolved.
4. web-app/src/test/setup.ts:23
- Draft comment:
Added mcpAutoApproveTools flag set to false. Consider adding an inline comment explaining its role. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% This is a test setup file where the purpose is to mock features for testing. The values themselves are just mocks, and their specific meanings are not particularly relevant in this context. Adding inline comments for individual flags would be unnecessary noise. The file already has a clear comment at the top explaining the purpose of these mocks. Perhaps the flag's meaning is not obvious to new developers, and documentation could help with maintainability. In a test setup file, the actual meaning of mock values is less important than in production code. The existing file-level comment on line 8-10 already explains the purpose of these mocks. The comment should be deleted as it suggests adding unnecessary documentation to test mock values.
5. web-app/src/test/setup.ts:24
- Draft comment:
Added mcpServersSettings flag with true. Ensure this fully replaces the removed mcpServers flag. - Reason this comment was not posted:
Comment looked like it was already resolved.
6. web-app/src/test/setup.ts:25
- Draft comment:
Added extensionsSettings flag set to true. Confirm this aligns with the PR description stating 'Extension settings are hidden'. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to confirm alignment with the PR description, which is against the rules. It doesn't provide a specific code suggestion or ask for a specific test to be written. Therefore, it should be removed.
Workflow ID: wflow_G04vpC091irv6XpT
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Describe Your Changes
Self Checklist
Important
Enable MCP auto-approval for web and disable certain settings pages, updating feature flags and settings menu accordingly.
MCP_AUTO_APPROVE_TOOLSinuseToolApproval.ts.MCP_SERVERS_SETTINGSandEXTENSIONS_SETTINGSfor web inconst.ts.mcp-servers.tsx.SettingsMenu.tsxto useMCP_SERVERS_SETTINGSandEXTENSIONS_SETTINGSflags.MCP_AUTO_APPROVE_TOOLS,MCP_SERVERS_SETTINGS, andEXTENSIONS_SETTINGStoPlatformFeatureenum intypes.ts.PlatformFeaturesinconst.tsto reflect new feature flags.setup.tsto mock new platform features for testing.This description was created by
for 4e633f0. You can customize this summary. It will automatically update as commits are pushed.