-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Ext/disk usage handle deleted files #24397
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?
Ext/disk usage handle deleted files #24397
Conversation
0eee29f to
d9638ec
Compare
|
Thank you for your contribution! 🎉 🔔 @easymikey you might want to have a look. You can use this guide to learn how to check out the Pull Request locally in order to test it. 📋 Quick checkout commandsBRANCH="ext/disk-usage-handle-deleted-files"
FORK_URL="https://github.com/easymikey/raycast-extensions.git"
EXTENSION_NAME="disk-usage"
REPO_NAME="raycast-extensions"
git clone -n --depth=1 --filter=tree:0 -b $BRANCH $FORK_URL
cd $REPO_NAME
git sparse-checkout set --no-cone "extensions/$EXTENSION_NAME"
git checkout
cd "extensions/$EXTENSION_NAME"
npm install && npm run devDue to our current reduced availability, the initial review may take up to 10-15 business days. |
Greptile OverviewGreptile SummaryThis PR attempts to fix the Architecture: The PR introduces a Critical Issues:
Unrelated Changes: The PR includes formatting changes to the brew extension and a committed Confidence Score: 0/5
Important Files ChangedFile Analysis
|
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.
5 files reviewed, 5 comments
| useEffect(() => { | ||
| if (!isExists) { | ||
| send({ type: "ITEM_MISSING", path: node.path, bytes: node.bytes }); | ||
| } | ||
| }, [node.path, isExists]); |
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.
[P0] This useEffect causes an infinite loop. When send({ type: "ITEM_MISSING" }) triggers refreshStore.triggerRefresh(), the component re-renders with a new node object, causing node.path to change reference and triggering the effect again. Additionally, send is missing from the dependency array, violating React's exhaustive-deps rule.
| const selection = useSelection(); | ||
| const isSelected = selection.has(node.path); | ||
| const isDeletingThis = isDeleting && isSelected; | ||
| const isExists = existsSync(node.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.
[P0] existsSync is called synchronously on every render. For a directory with hundreds of files, this will block the UI thread with hundreds of synchronous filesystem checks. This defeats the purpose of React's rendering model and will cause severe performance degradation.
| useEffect(() => { | ||
| if (!isExists) { | ||
| send({ type: "ITEM_MISSING", path: node.path, bytes: node.bytes }); | ||
| } | ||
| }, [node.path, isExists]); |
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.
[P1] The approach of checking file existence on every render is architecturally flawed. The ITEM_MISSING event should be sent only when an actual deletion attempt fails (catching ENOENT errors in the trash operation), not proactively checked during render.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Additional Comments (2)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! |
|
Thanks for your update @easymikey 🔥 Can you see feedback from Greptile? I converted this PR into a draft until it's ready for the review, please press the button |
Description
Fix file deletion if they have already been deleted
Checklist
npm run buildand tested this distribution build in Raycastassetsfolder are used by the extension itselfREADMEare located outside the metadata folder if they were not generated with our metadata tool