-
Notifications
You must be signed in to change notification settings - Fork 2.3k
feat: proxy support for the new downloader #5795
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
feat: proxy support for the new downloader #5795
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 4eaeb32 in 2 minutes and 30 seconds. Click for details.
- Reviewed
1531lines of code in9files - Skipped
0files when reviewing. - Skipped posting
9draft 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. extensions/download-extension/src/index.ts:31
- Draft comment:
Good integration: proxy config is now passed to downloadFiles in downloadFile. Consider documenting the expected proxy config shape in a code comment for clarity. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. extensions/llamacpp-extension/src/backend.ts:129
- Draft comment:
Proxy config is integrated into backend download items. Ensure that getProxyConfig behavior is consistent across extensions. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
3. extensions/llamacpp-extension/src/index.ts:611
- Draft comment:
Proxy config is now passed in model downloads. This is consistent; ensure any proxy setting changes update the UI accordingly. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
4. extensions/llamacpp-extension/src/util.test.ts:40
- Draft comment:
Comprehensive tests for getProxyConfig. Consider extracting common mock setup into a shared test helper to reduce repetition. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
5. extensions/llamacpp-extension/src/util.ts:64
- Draft comment:
Test expecting console.log output: getProxyConfig does not log any details. Either add logging or update the test accordingly. - 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% The comment seems to misunderstand the code - there is logging where it matters (error cases). The function is a utility that returns a config object, it doesn't need to log successful operations. The comment appears to be making assumptions about test requirements without strong evidence. We don't have context about what test they're referring to. Maybe there is a test somewhere that expects console.log output that I can't see? Maybe there's a logging standard in the codebase that requires logging successful operations? Without seeing the test in question or documentation about logging standards, we can't verify if this comment is valid. The function already logs errors appropriately. The comment should be deleted as it makes assumptions about testing/logging requirements without clear evidence, and the function already has appropriate error logging.
6. src-tauri/src/core/utils/download.rs:49
- Draft comment:
Proxy validation functions are well implemented. Consider caching the parsed URL to avoid double parsing in validate_proxy_config. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
7. src-tauri/src/core/utils/download.rs:160
- Draft comment:
Using proxy_config.no_proxy.as_deref().unwrap_or(&[]) is clear. Ensure that the wildcard matching in should_bypass_proxy meets enterprise requirements. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
8. web-app/src/routes/settings/https-proxy.tsx:44
- Draft comment:
Simplified proxy settings UI by removing calls to configurePullOptions; verify that downstream consumers pick up proxy settings from localStorage. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
9. web-app/src/services/models.ts:28
- Draft comment:
No specific changes regarding proxy support here; just ensure model operations are unaffected by proxy configuration changes. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_Yf2n091BKdZpThSO
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.
Important
Looks good to me! 👍
Reviewed b7be879 in 1 minute and 1 seconds. Click for details.
- Reviewed
37lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft 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/services/__tests__/models.test.ts:247
- Draft comment:
Removed tests for 'configurePullOptions'. Ensure that proxy configuration functionality is covered elsewhere to maintain test coverage. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment suggests ensuring that the proxy configuration functionality is covered elsewhere, which is a form of asking the author to ensure something is tested. This violates the rule against asking the author to ensure the change is tested.
Workflow ID: wflow_8I9rnxIguoi2GGb5
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Barecheck - Code coverage reportTotal: 35.35%Your code coverage diff: 0.03% ▴ |
b7be879 to
b76b1d8
Compare
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.
So far LGTM.
85495ca to
b7be879
Compare
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 b5ab1ae in 1 minute and 35 seconds. Click for details.
- Reviewed
37lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft 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. .github/workflows/jan-linter-and-test.yml:114
- Draft comment:
Removed redundant 'cargo install tauri-driver --locked' steps. Please confirm that tauri-driver is no longer needed in these jobs or is installed elsewhere to avoid breaking any tests. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_fD0RI9wdrTOkwoAN
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Add proxy support for the new downloader
Summary
Changes
ProxyConfigstruct with SSL verification and no-proxy list supportCleanShot.2025-07-17.at.00.21.39.mp4
Test plan
Important
Added proxy support for the Rust-based downloader, integrated proxy settings into llamacpp extension, and updated web app with comprehensive tests.
ProxyConfigstruct indownload.rswith SSL verification and no-proxy list support.llamacpp-extension.util.test.tsfor proxy configuration handling.download.rsfor proxy validation and client creation.tauri-driverinstallation fromjan-linter-and-test.yml.urldependency inCargo.tomlfor URL parsing.This description was created by
for b5ab1ae. You can customize this summary. It will automatically update as commits are pushed.