-
Notifications
You must be signed in to change notification settings - Fork 3.8k
adding needed changes #18987
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
adding needed changes #18987
Conversation
Thank you for the update! 🎉 You can expect an initial review within five business days. |
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 introduces significant improvements to the Ente Auth extension, focusing on path resolution, icon handling, and import/export functionality.
- The changelog entry for bug fix needs
{PR_MERGE_DATE}
instead of hardcoded date2024-11-14
- The command title change from "Get TOTP" to "Get Totp" in
package.json
should be kept as "Get TOTP" since it's the unique ID - Remove example usage code at bottom of
/src/helpers/icons.ts
(lines 70-75) showToast
in/src/export.tsx
should be replaced withshowFailureToast
from@raycast/utils
for better error handling- The
any
type in icon matching function in/src/helpers/icons.ts
should be properly typed for type safety
💡 (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!
13 file(s) reviewed, 15 comment(s)
Edit PR Review Bot Settings | Greptile
import path from "path"; | ||
|
||
export const DEFAULT_EXPORT_DIR_PATH = (): string => { | ||
const exportPath = getPreferenceValues().exportPath || path.join(process.env.HOME || "", "Documents", "ente"); |
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: process.env.HOME may be undefined on Windows. Consider using os.homedir() as the primary source
const exportPath = getPreferenceValues().exportPath || path.join(process.env.HOME || "", "Documents", "ente"); | |
const exportPath = getPreferenceValues().exportPath || path.join(os.homedir(), "Documents", "ente"); |
|
||
export const EXPORTPATH = getPreferenceValues().exportPath || `${DEFAULT_EXPORT_PATH}/ente_auth.txt`; | ||
export const EXPORT_FILE_PATH = `${DEFAULT_EXPORT_DIR_PATH()}/ente_auth.txt`; |
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: DEFAULT_EXPORT_DIR_PATH() is called on every reference - consider memoizing the result
@@ -1,5 +1,8 @@ | |||
# Ente Auth Changelog | |||
|
|||
## [Bug fix] - 2024-11-14 |
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.
syntax: The title should use the {PR_MERGE_DATE} placeholder instead of a hardcoded date. Change to ## [Bug fix] - {PR_MERGE_DATE}
## [Bug fix] - 2024-11-14 | |
## [Bug fix] - {PR_MERGE_DATE} |
extensions/ente-auth/package.json
Outdated
@@ -25,14 +25,15 @@ | |||
"commands": [ | |||
{ | |||
"name": "index", | |||
"title": "Get TOTP", | |||
"title": "Get Totp", |
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 case inconsistency - 'TOTP' was changed to 'Totp' which is less standard
"title": "Get Totp", | |
"title": "Get TOTP", |
extensions/ente-auth/src/export.tsx
Outdated
} catch (error) { | ||
showToast(Toast.Style.Failure, "Folder creation failed"); | ||
return <Detail markdown={`## Failed to create folder at \`${EXPORTPATH}\``} />; | ||
return <Detail markdown={`## Failed to create folder at \`${DEFAULT_EXPORT_DIR_PATH}\``} />; |
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: DEFAULT_EXPORT_DIR_PATH is a function but missing () here - will display function definition instead of path
return <Detail markdown={`## Failed to create folder at \`${DEFAULT_EXPORT_DIR_PATH}\``} />; | |
return <Detail markdown={`## Failed to create folder at \`${DEFAULT_EXPORT_DIR_PATH()}\``} />; |
if (icons[key as keyof typeof icons].slug === serviceName) { | ||
return icons[key as keyof typeof icons].svg; | ||
} |
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: Use Object.entries() or Object.keys() instead of for...in to avoid iterating over prototype chain
if (!checkEnteBinary()) { | ||
showToast(Toast.Style.Failure, "Ente binary not found"); | ||
} |
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: The function continues execution after showing the binary not found error. This should return early to prevent further operations.
if (!checkEnteBinary()) { | |
showToast(Toast.Style.Failure, "Ente binary not found"); | |
} | |
if (!checkEnteBinary()) { | |
showToast(Toast.Style.Failure, "Ente binary not found"); | |
return; | |
} |
createEntePath(DEFAULT_EXPORT_DIR_PATH()); | ||
exportEnteAuthSecrets(); |
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: These operations could fail but their errors aren't being caught. They should be inside the try block or have their own error handling.
style: Toast.Style.Animated, | ||
title: "Importing secrets", | ||
}); | ||
const toast = await showToast({ style: Toast.Style.Animated, title: "Importing secrets" }); |
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: Multiple toast notifications could cause race conditions. Consider using showFailureToast from @raycast/utils for error cases.
extensions/ente-auth/src/import.tsx
Outdated
if (getPreferenceValues().deleteExport == true) { | ||
deleteEnteExport(); | ||
}; |
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: deleteEnteExport() could fail but errors are not handled. This should be in a try-catch block.
Hi @chkpwd👋 Thanks for your contribution 🔥 Could you look into the merge conflicts |
Hey @pernielsentikaer, thanks for all your hard work! Done. |
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.
Hi 👋
Looks good to me, approved 🔥
Published to the Raycast Store: |
🎉 🎉 🎉 We've rewarded your Raycast account with some credits. You will soon be able to exchange them for some swag. |
Description
This update introduces several improvements and fixes across the extension:
import
command now performs all necessary setup and configuration actions automatically (d94ddeabc3c8
).ab363f78ab6d
).7d077481b83a
).c4faec441f78
).386fd572c2fa
).781b6e5df02d
).stripServiceName
to better handle edge cases (8dcf2826f9eb
).28259c72872e
).0028da00ab27
).4befca5a81ae
).1515dfa6e494
,23e7dcc9ba7a
).Checklist
npm run build
and tested this distribution build in Raycastassets
folder are used by the extension itselfREADME
are located outside the metadata folder if they were not generated with our metadata tool