-
Notifications
You must be signed in to change notification settings - Fork 9
Support Profile Detection for the Auto Profiles (#42) #50
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
Conversation
Summary of ChangesHello @SYM01, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the 'Auto Switch' proxy mode by introducing intelligent, per-tab profile detection. It allows the extension to automatically identify and display the proxy profile being used for the active browser tab based on a set of configurable rules. This feature aims to provide users with greater transparency and control over their proxy settings in dynamic browsing environments. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request introduces a new feature to display per-tab proxy usage statistics and detection information, particularly for 'auto-switch' proxy profiles. Key changes include adding activeTab permission to manifest.json, updating numerous dependencies in package.json, and introducing new internationalization messages for auto-mode detection. The core functionality involves refactoring browser adapter types and methods (src/adapters/base.ts, chrome.ts, firefox.ts, web.ts) to support per-tab badge updates, tab removal events, and inter-extension messaging. A new WebRequestStatsService is implemented in src/services/stats.ts to track failed requests and the currently detected proxy profile for each tab, utilizing a new pacSimulator.ts for PAC script evaluation logic. The background.ts script is updated to initialize this stats service, listen for web request events (response started, errors, tab removal), and dynamically update the browser action badge for specific tabs. A new AutoModeActionBar.vue component is added to the popup UI (PopupPage.vue) to display the detected proxy profile for the active tab when in auto-switch mode. Review comments highlight issues with the onWebRequestAuthRequired signature in base.ts and its implementations, suggesting a promise-based approach for better handling of async operations and fixing a critical proxy authentication bug. Additionally, several test cases in profile2config.test.ts are identified as having incorrect expectations regarding how findProfile should behave for non-auto and auto-switch profiles, and a setTimeout workaround in background.ts is flagged for needing a link to the relevant Chrome bug report. A block of commented-out mocking code in AutoModeActionBar.vue is also noted for removal.
| abstract onWebRequestAuthRequired( | ||
| callback: ( | ||
| details: WebAuthenticationChallengeDetails, | ||
| callback?: (response: BlockingResponse) => void | ||
| ) => void | ||
| asyncCallback?: (response: BlockingResponse) => void | ||
| ) => BlockingResponse | undefined | ||
| ): void; |
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 signature of onWebRequestAuthRequired is overly complex and attempts to support both Manifest V2 (callback-based) and V3 (promise-based) async models in a way that is causing bugs. A cleaner approach is to use a promise-based signature, which is supported in both modern Chrome and Firefox.
This change is part of a fix for a critical bug in proxy authentication. Please see the related comments in src/background.ts, src/adapters/chrome.ts, and src/adapters/firefox.ts.
abstract onWebRequestAuthRequired(
callback: (
details: WebAuthenticationChallengeDetails
) => Promise<BlockingResponse | undefined>
): void;| onWebRequestAuthRequired( | ||
| callback: ( | ||
| details: WebAuthenticationChallengeDetails, | ||
| callback?: (response: BlockingResponse) => void | ||
| ) => void | ||
| asyncCallback?: (response: BlockingResponse) => void | ||
| ) => BlockingResponse | undefined | ||
| ): void { | ||
| browser.webRequest.onAuthRequired.addListener( | ||
| callback, | ||
| callback as any, | ||
| { urls: ["<all_urls>"] }, | ||
| ["asyncBlocking"] | ||
| ); | ||
| } |
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.
Following the proposed change in base.ts to use a promise-based callback for onWebRequestAuthRequired, this implementation can be simplified. The asyncBlocking option allows the listener to return a promise, and the as any cast is no longer needed. This fixes a critical bug in proxy authentication.
onWebRequestAuthRequired(
callback: (
details: WebAuthenticationChallengeDetails
) => Promise<BlockingResponse | undefined>
): void {
browser.webRequest.onAuthRequired.addListener(
callback,
{ urls: ["<all_urls>"] },
["asyncBlocking"]
);
}| test("auto profile with domain rule match", async () => { | ||
| const profile = new ProfileConverter(profiles.autoProxy, profileLoader); | ||
| const url = new URL("https://test.example.com"); | ||
| const result = await profile.findProfile(url); | ||
|
|
||
| // When a rule matches a proxy profile, findProfile on proxy returns undefined | ||
| // So it falls back to default profile | ||
| expect(result.profile).toBeDefined(); | ||
| const config = await result.profile!.toProxyConfig(); | ||
| expect(config.mode).toBe("direct"); // Falls back to default | ||
| expect(result.isConfident).toBe(true); | ||
| }); |
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 test's expectation is incorrect based on the implementation of findProfile. When a rule in an auto-switch profile matches, findProfile should return the matched sub-profile, not fall back to the default profile. Here, https://test.example.com matches the *.example.com rule, which points to simpleProxy. The result should be the simpleProxy profile. The test incorrectly expects the direct (default) profile.
test("auto profile with domain rule match", async () => {
const profile = new ProfileConverter(profiles.autoProxy, profileLoader);
const url = new URL("https://test.example.com");
const result = await profile.findProfile(url);
expect(result.profile).toBeDefined();
expect(result.profile!.profile.profileID).toBe("simpleProxy");
expect(result.isConfident).toBe(true);
});| test("proxy profile (non-auto) should return undefined", async () => { | ||
| const profile = new ProfileConverter(profiles.simpleProxy); | ||
| const url = new URL("https://example.com"); | ||
| const result = await profile.findProfile(url); | ||
|
|
||
| // Proxy profiles are not handled by findProfile, should return undefined | ||
| expect(result.profile).toBeUndefined(); | ||
| expect(result.isConfident).toBe(true); | ||
| }); | ||
| }); |
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 test has an incorrect expectation. The findProfile method is designed to find the terminal profile to use for a given URL. For a non-auto profile (like a simple proxy), it should return itself, as it is the terminal profile. The implementation correctly returns { profile: this, isConfident: true }. However, this test expects result.profile to be undefined. This test should be corrected to reflect the implementation's logic. Several other tests in this file seem to be based on this wrong assumption and should also be reviewed.
test("proxy profile (non-auto) should return itself", async () => {
const profile = new ProfileConverter(profiles.simpleProxy);
const url = new URL("https://example.com");
const result = await profile.findProfile(url);
expect(result.profile).toBe(profile);
expect(result.isConfident).toBe(true);
});|
|
||
| if (ret.profile && ret.isConfident) { | ||
| const profile = ret.profile.profile; | ||
| setTimeout(() => setIndicator(profile, details.tabId), 50); // Sometimes the indicator doesn't update properly in Chrome, so we need to wait a bit. |
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.
Using setTimeout with a magic number like 50 is a code smell and can lead to race conditions or flaky behavior. The comment mentions it's a workaround for a Chrome bug. For better maintainability, please add a link to the corresponding Chrome bug report. This will provide context for future developers and help track if/when the workaround can be removed.
| // mocking | ||
| // tabStats.value = { | ||
| // failedRequests: [], | ||
| // currentProfile: { | ||
| // profile: { | ||
| // profileID: "profile1", | ||
| // profileName: "Profile 1", | ||
| // proxyType: "proxy", | ||
| // color: "#000000", | ||
| // proxyRules: { | ||
| // default: { | ||
| // host: "127.0.0.1", | ||
| // port: 8080, | ||
| // }, | ||
| // bypassList: [], | ||
| // }, | ||
| // }, | ||
| // isConfident: true, | ||
| // }, | ||
| // tabID: props.currentTab.id, | ||
| // }; | ||
| // console.log(tabStats.value); | ||
| }; |
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.
* update github action * minor change to System profile * Build(deps): Bump nanoid from 3.3.7 to 3.3.8 (#21) Bumps [nanoid](https://github.com/ai/nanoid) from 3.3.7 to 3.3.8. - [Release notes](https://github.com/ai/nanoid/releases) - [Changelog](https://github.com/ai/nanoid/blob/main/CHANGELOG.md) - [Commits](ai/nanoid@3.3.7...3.3.8) --- updated-dependencies: - dependency-name: nanoid dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Avoid blocking 401 authentication request (#22) (#23) * fix 401 issue (#22) * enablle github action on develop branch * Support Firefox (#18) * Adapt firefox * support firefore auto publish * support firefox, and optimized UX * [i18n] Updates for file public/_locales/en/messages.json (#25) * [i18n] Translate messages.json in pt_BR 100% reviewed source file: 'messages.json' on 'pt_BR'. * [i18n] Translate messages.json in zh_CN 100% reviewed source file: 'messages.json' on 'zh_CN'. * [i18n] Translate messages.json in zh_TW 100% reviewed source file: 'messages.json' on 'zh_TW'. * [i18n] Translate messages.json in pt_BR 100% reviewed source file: 'messages.json' on 'pt_BR'. * update English i18n msg --------- Co-authored-by: transifex-integration[bot] <43880903+transifex-integration[bot]@users.noreply.github.com> Co-authored-by: SYM01 <33443792+SYM01@users.noreply.github.com> * sentry integration (#29) * [WIP] support exporting * [WIP] support import/export settings (#7) * Support import/export profiles and close #7 (#30) * implemented an utility to export/import settings * optimized description * [i18n] Updates for file public/_locales/en/messages.json (#32) * [i18n] Translate messages.json in pt_BR 100% reviewed source file: 'messages.json' on 'pt_BR'. * [i18n] Translate messages.json in zh_TW 100% reviewed source file: 'messages.json' on 'zh_TW'. * [i18n] Translate messages.json in zh_CN 100% reviewed source file: 'messages.json' on 'zh_CN'. --------- Co-authored-by: transifex-integration[bot] <43880903+transifex-integration[bot]@users.noreply.github.com> * fix an import issue (#33) (#35) * optimized sentry release * Optimize Development Flow (#37) * [WIP] support chrome auto deploy * finalize Chrome auto deploy * npm audit fix * npm audit fix * UX improvement and close #28 (#38) * minor bug fixed * improve UX and close #28 * use the same CIDR validator as PAC script * [i18n] Updates for file public/_locales/en/messages.json (#40) * [i18n] Translate messages.json in pt_BR 100% reviewed source file: 'messages.json' on 'pt_BR'. * [i18n] Translate messages.json in zh_TW 100% reviewed source file: 'messages.json' on 'zh_TW'. * [i18n] Translate messages.json in zh_CN 100% reviewed source file: 'messages.json' on 'zh_CN'. --------- Co-authored-by: transifex-integration[bot] <43880903+transifex-integration[bot]@users.noreply.github.com> * Support Authentication in Auto Switch Profiles, fixing #34 (#45) * update deps * fix the auth issue in auto switch profiles * fix typo * Support Profile Detection for the Auto Profiles (#42) (#50) * [WIP] Reflect current profile when using auto profile (#42) * Show detected profile in the popup * simplify test cases --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: transifex-integration[bot] <43880903+transifex-integration[bot]@users.noreply.github.com>
* update github action * minor change to System profile * Build(deps): Bump nanoid from 3.3.7 to 3.3.8 (#21) Bumps [nanoid](https://github.com/ai/nanoid) from 3.3.7 to 3.3.8. - [Release notes](https://github.com/ai/nanoid/releases) - [Changelog](https://github.com/ai/nanoid/blob/main/CHANGELOG.md) - [Commits](ai/nanoid@3.3.7...3.3.8) --- updated-dependencies: - dependency-name: nanoid dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Avoid blocking 401 authentication request (#22) (#23) * fix 401 issue (#22) * enablle github action on develop branch * Support Firefox (#18) * Adapt firefox * support firefore auto publish * support firefox, and optimized UX * [i18n] Updates for file public/_locales/en/messages.json (#25) * [i18n] Translate messages.json in pt_BR 100% reviewed source file: 'messages.json' on 'pt_BR'. * [i18n] Translate messages.json in zh_CN 100% reviewed source file: 'messages.json' on 'zh_CN'. * [i18n] Translate messages.json in zh_TW 100% reviewed source file: 'messages.json' on 'zh_TW'. * [i18n] Translate messages.json in pt_BR 100% reviewed source file: 'messages.json' on 'pt_BR'. * update English i18n msg --------- Co-authored-by: transifex-integration[bot] <43880903+transifex-integration[bot]@users.noreply.github.com> Co-authored-by: SYM01 <33443792+SYM01@users.noreply.github.com> * sentry integration (#29) * [WIP] support exporting * [WIP] support import/export settings (#7) * Support import/export profiles and close #7 (#30) * implemented an utility to export/import settings * optimized description * [i18n] Updates for file public/_locales/en/messages.json (#32) * [i18n] Translate messages.json in pt_BR 100% reviewed source file: 'messages.json' on 'pt_BR'. * [i18n] Translate messages.json in zh_TW 100% reviewed source file: 'messages.json' on 'zh_TW'. * [i18n] Translate messages.json in zh_CN 100% reviewed source file: 'messages.json' on 'zh_CN'. --------- Co-authored-by: transifex-integration[bot] <43880903+transifex-integration[bot]@users.noreply.github.com> * fix an import issue (#33) (#35) * optimized sentry release * Optimize Development Flow (#37) * [WIP] support chrome auto deploy * finalize Chrome auto deploy * npm audit fix * npm audit fix * UX improvement and close #28 (#38) * minor bug fixed * improve UX and close #28 * use the same CIDR validator as PAC script * [i18n] Updates for file public/_locales/en/messages.json (#40) * [i18n] Translate messages.json in pt_BR 100% reviewed source file: 'messages.json' on 'pt_BR'. * [i18n] Translate messages.json in zh_TW 100% reviewed source file: 'messages.json' on 'zh_TW'. * [i18n] Translate messages.json in zh_CN 100% reviewed source file: 'messages.json' on 'zh_CN'. --------- Co-authored-by: transifex-integration[bot] <43880903+transifex-integration[bot]@users.noreply.github.com> * Support Authentication in Auto Switch Profiles, fixing #34 (#45) * update deps * fix the auth issue in auto switch profiles * fix typo * Support Profile Detection for the Auto Profiles (#42) (#50) * [WIP] Reflect current profile when using auto profile (#42) * Show detected profile in the popup * simplify test cases * [i18n] Updates for file public/_locales/en/messages.json (#52) * [i18n] Translate messages.json in zh_TW 100% reviewed source file: 'messages.json' on 'zh_TW'. * [i18n] Translate messages.json in zh_CN 100% reviewed source file: 'messages.json' on 'zh_CN'. * [i18n] Translate messages.json in pt_BR 100% reviewed source file: 'messages.json' on 'pt_BR'. --------- Co-authored-by: transifex-integration[bot] <43880903+transifex-integration[bot]@users.noreply.github.com> --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: transifex-integration[bot] <43880903+transifex-integration[bot]@users.noreply.github.com>
No description provided.