-
Notifications
You must be signed in to change notification settings - Fork 2.1k
fix: improve proxy connection error detection and documentation #7059
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
base: main
Are you sure you want to change the base?
Conversation
- Add proxy configuration detection utility - Show warning when http.electronFetch is disabled with proxy settings - Improve error messages to guide users to the solution - Add comprehensive proxy configuration documentation - Update README with link to proxy documentation Fixes #6991
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.
Reviewing my own code is like debugging in a mirror - everything looks backwards but the bugs are still mine.
await vscode.env.openExternal(vscode.Uri.parse("https://github.com/microsoft/vscode/issues/12588")) | ||
} | ||
} | ||
} |
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.
This new utility file lacks test coverage. Consider adding unit tests to verify:
- Detection logic for various proxy configurations
- Error message formatting for different error types
- Warning notification behavior
You could create src/api/providers/utils/__tests__/proxy-detection.spec.ts
following the pattern in timeout-config.spec.ts
.
const args = process.argv.join(" ") | ||
const hasPacFile = args.includes("--proxy-pac-url") | ||
if (hasPacFile) { | ||
const pacMatch = args.match(/--proxy-pac-url[= ]([^ ]+)/) |
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.
This regex pattern might not handle all edge cases. For example, it won't correctly parse:
- Quoted URLs with spaces:
--proxy-pac-url="http://proxy.com/pac file.js"
- URLs with special characters that need escaping
Consider using a more robust pattern or documenting the expected format:
src/extension.ts
Outdated
@@ -81,6 +82,9 @@ export async function activate(context: vscode.ExtensionContext) { | |||
// Initialize i18n for internationalization support. | |||
initializeI18n(context.globalState.get("language") ?? formatLanguage(vscode.env.language)) | |||
|
|||
// Check for proxy configuration issues and show warning if needed | |||
await showProxyConfigurationWarning() |
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.
Is the fire-and-forget approach intentional here? If a user quickly dismisses the warning and tries to use the extension, they might encounter issues before the warning's action completes. Consider whether this should be awaited or if the current approach is sufficient for the use case.
|
||
1. **Verify proxy settings**: Ensure your proxy configuration is correct | ||
2. **Check firewall rules**: Make sure VSCode is allowed through your firewall | ||
3. **Test connectivity**: Try accessing the API endpoint directly via curl or browser |
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.
The documentation mentions using curl for testing connectivity but doesn't provide example commands. Consider adding specific examples to help users troubleshoot more effectively:
bash
Test OpenAI API
curl -H "Authorization: Bearer YOUR_API_KEY" https://api.openai.com/v1/models
Test with proxy
curl -x http://your-proxy:8080 https://api.openai.com/v1/models
} | ||
|
||
const hasProxyConfig = proxySettings.length > 0 | ||
const hasIssue = hasProxyConfig && !electronFetchEnabled |
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.
The detection only checks if http.electronFetch
is disabled but doesn't verify if the proxy settings themselves are valid. Users might still have issues if their proxy configuration is incorrect. Consider adding validation for common proxy configuration issues or mentioning this limitation in the error message.
@roomote-agent trying to detect proxy usage seems fragile. Is there a method of just always using the Electron fetch implementation instead of the Node.js implementation? |
Hi @bstrdsmkr! Thanks for the feedback. I understand your concern about the fragility of proxy detection. Let me investigate the possibility of always using the Electron fetch implementation instead of Node.js fetch. I'll explore the codebase to understand the implications and provide a solution. |
- Remove automatic proxy detection logic from extension startup - Remove proxy-detection.ts utility file - Simplify error messages to suggest enabling http.electronFetch when connection issues occur - Update documentation to focus on the solution rather than detection As suggested by @bstrdsmkr, the proxy detection approach was fragile. VSCode extensions cannot directly use Electron fetch - they can only suggest users enable the http.electronFetch setting which tells VSCode itself to use Electron fetch internally.
errorMsg.includes("ENOTFOUND") | ||
) { | ||
errorMsg += | ||
"\n\nIf you're behind a proxy, try enabling the 'http.electronFetch' setting in VSCode:\n1. Open Settings (Cmd/Ctrl + ,)\n2. Search for 'http.electronFetch'\n3. Enable the setting\n4. Restart VSCode" |
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.
Consider using the i18n translation function (e.g., t()) for the proxy-related error message, instead of an inline hard-coded string. This ensures consistent localization for user-facing messages.
This comment was generated because it violated a code review rule: irule_C0ez7Rji6ANcGkkX.
Hi @bstrdsmkr! I've implemented your suggestion to remove the fragile proxy detection logic. Here's what I changed: Changes Made
Key InsightYou were absolutely right - VSCode extensions cannot directly use Electron's fetch implementation. They run in a Node.js environment and can only use the Node.js fetch. The The best we can do is:
This approach is much cleaner and less fragile than trying to detect proxy configurations. Thank you for the feedback! |
Summary
This PR addresses the issue reported in #6991 where users experience connection errors when using proxies with the
http.electronFetch
setting disabled (which is the default in VSCode).Problem
When VSCode is configured to use a proxy (SOCKS5, HTTP proxy, or PAC file) but the
http.electronFetch
setting is disabled, the extension fails to connect to API endpoints. This particularly affects OpenAI and OpenAI-compatible providers that use the native fetch API.Solution
http.electronFetch
is disabledChanges
src/api/providers/utils/proxy-detection.ts
with proxy detection utilitiessrc/core/task/Task.ts
to use enhanced error messagessrc/extension.ts
to show warning on startupdocs/PROXY_CONFIGURATION.md
with detailed proxy setup guideREADME.md
with link to proxy documentationTesting
Related Issues
Fixes #6991
Notes for Reviewers
The core issue is that VSCode has two different fetch implementations:
This PR doesn't change the default behavior but provides clear guidance to users on how to resolve the issue by enabling
http.electronFetch
.cc @bstrdsmkr - This PR implements the improvements you suggested. The extension will now detect the proxy configuration issue and provide helpful guidance to users.
Important
Improves proxy connection error handling in VSCode by adding detection utilities, warning notifications, enhanced error messages, and comprehensive documentation.
proxy-detection.ts
to identify whenhttp.electronFetch
is disabled.extension.ts
if proxy settings are detected withhttp.electronFetch
disabled.Task.ts
to suggest enablinghttp.electronFetch
for proxy-related errors.PROXY_CONFIGURATION.md
for detailed proxy setup instructions.README.md
with a link to the new proxy configuration guide.This description was created by
for d25f7bb. You can customize this summary. It will automatically update as commits are pushed.