-
Notifications
You must be signed in to change notification settings - Fork 4k
Update mozilla-vpn extension #19223
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
Update mozilla-vpn extension #19223
Conversation
- Fixing Title Cased - Updated the screenshots - Merge branch \'contributions/merge-1747357386139\' into server-select - Pull contributions - code formatting - Merge pull request #1 from nwhistler/server-select
Thank you for the update! 🎉 You can expect an initial review within five business days. |
solving #17926 |
Remember to make as ready for review when it's ready and see the suggestions from Greptile, @nwhistler |
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.
PR Summary
This PR adds comprehensive server selection functionality to the Mozilla VPN extension, with improved error handling and UI components for managing VPN connections and server locations.
- CHANGELOG.md title should be 'Mozilla Vpn Connect' to match package.json and entry date should use
{PR_MERGE_DATE}
instead of hardcoded '2024-04-26' toggleVpn.tsx
should useshowFailureToast
from@raycast/utils
instead of manual error handlingserverSelector.tsx
relies on external API (countryflagsapi.com) without proper fallback for flag imagesfetchCurrentIP.ts
has multiple timeout handlers that could potentially conflictmetadata
folder missing for view command screenshots
💡 (3/5) Reply to the bot's comments like "Can you suggest a fix for this @greptileai?" or ask follow-up questions!
13 file(s) reviewed, 24 comment(s)
Edit PR Review Bot Settings | Greptile
@@ -1,3 +1,3 @@ | |||
# Mozilla VPN Connect Changelog | |||
# Moz VPN Changelog |
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.
style: Title should match package.json title 'Mozilla Vpn Connect' for consistency
@@ -19,15 +25,29 @@ const openMozillaVpnApp = () => { | |||
}); | |||
}; | |||
|
|||
const CheckLogin: React.FC = () => { | |||
const CheckLogin: React.FC<CheckLoginProps> = ({ onBack }) => { | |||
return ( | |||
<List.EmptyView | |||
title="Mozilla VPN Login Required" | |||
description="Please log in using the Mozilla VPN application. Press Enter to open the app." | |||
icon="⚠️" |
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.
style: Consider using Icon.ExclamationMark instead of emoji for consistency with Raycast's design system
Select a server, must be the actual server name and not the city `/Applications/Mozilla\ VPN.app/Contents/MacOS/Mozilla\ VPN select` | ||
|
||
|
||
|
||
Status shows if the client is connected or not, and the server city and country it is configured for. There are other details here like all devices configured for the service. |
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.
style: This line should not be indented with spaces as it breaks the markdown formatting
Status shows if the client is connected or not, and the server city and country it is configured for. There are other details here like all devices configured for the service. | |
Status shows if the client is connected or not, and the server city and country it is configured for. There are other details here like all devices configured for the service. |
if (!statusFailed && vpnStatus !== null) { | ||
// If VPN is active, wait a bit longer for the connection to stabilize | ||
const waitTime = vpnStatus ? 2000 : 1000; | ||
await new Promise((resolve) => setTimeout(resolve, waitTime)); |
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.
style: Potential race condition - vpnStatus state may have changed since statusFailed check. Consider capturing status in a local variable.
// Log the full output for debugging | ||
console.log(`Raw VPN status output:\n${stdout}`); |
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.
logic: Logging raw VPN output could expose sensitive information. Consider redacting sensitive data before logging
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Description
Screencast
Checklist
npm run build
and tested this distribution build in Raycastassets
folder are used by the extension itselfREADME
are placed outside of themetadata
folder