Conversation
|
Thank you for the update! 🎉 You can expect an initial review within five business days. |
There was a problem hiding this comment.
PR Summary
This PR updates the 1bookmark extension by removing jotai dependency and implementing performance optimizations to prevent unnecessary re-renders in the Raycast environment.
- Replaced jotai atoms with
useCachedStatefrom@raycast/utilsacross multiple components for more efficient state management - Added new
useLoggedOutStatushook to centralize authentication state and cache clearing logic - Introduced
useTagshook with proper caching to optimize tag data fetching and management - Implemented extensive memoization using
useMemoin browser bookmark hooks to prevent unnecessary recalculations - Extracted TRPC client setup and error handling into separate utility files for better code organization
The changes appear well-structured and follow Raycast's guidelines, focusing on performance optimization while maintaining functionality.
24 file(s) reviewed, 6 comment(s)
Edit PR Review Bot Settings | Greptile
| const folders = useMemo(() => { | ||
| return root.filter((item) => containerIds.includes(item.parentID ?? "")).flatMap((item) => getFolders(root, item)); | ||
| }, [root, containerIds]); |
There was a problem hiding this comment.
style: The folders calculation depends on root.filter() result but root itself is already memoized. Consider moving the filter operation inside the root useMemo to reduce the number of memoized values.
There was a problem hiding this comment.
This code review is not a good suggestion. Here's why:
Separation of Concerns
- root is responsible for simply fetching all bookmark items.
- folders is responsible for creating the folder structure for the current profile using containerIds.
- Separating these two logics improves code readability and maintainability.
Dependency Management
- root only depends on container.
- folders depends on both root and containerIds.
- If we include filtering in root, it would make root depend on containerIds as well, making dependencies more complex.
Reusability
- The current structure allows root to provide pure bookmark items that can be used without filtering in other places if needed.
- If we include filtering in root, we would have to use filtered results even when filtering is not needed.
Performance
- root is only recalculated when container changes.
- folders is only recalculated when either root or containerIds changes.
- The current structure provides more efficient memoization.
Therefore, maintaining the current code is a better choice. The code review suggestion seems to be focused on reducing code lines, but it could actually harm the code's structure and maintainability.
| useEffect(() => { | ||
| if (!r.data) return; | ||
|
|
||
| cache.set("bookmarks", JSON.stringify(r.data)); | ||
| setCached(r.data); | ||
| }, [r.data]); |
There was a problem hiding this comment.
logic: Missing dependency array items. setCached should be included in the useEffect dependencies to prevent stale closure issues.
| useEffect(() => { | |
| if (!r.data) return; | |
| cache.set("bookmarks", JSON.stringify(r.data)); | |
| setCached(r.data); | |
| }, [r.data]); | |
| useEffect(() => { | |
| if (!r.data) return; | |
| setCached(r.data); | |
| }, [r.data, setCached]); |
There was a problem hiding this comment.
I applied the code you suggested.
| import { RouterOutputs, trpc } from "@/utils/trpc.util"; | ||
| import { cache } from "../views/MyAccount"; |
There was a problem hiding this comment.
style: importing cache from a view file (MyAccount) could create circular dependencies - consider moving cache to a separate utils file
There was a problem hiding this comment.
I don't think it's important because I only use it in MyAccount and here. I'll consider it when I use it in more places.
| if (now.getTime() - signOutDate.getTime() < 1000) { | ||
| setAfter1Sec(false); | ||
| setTimeout(() => setAfter1Sec(true), 1000); | ||
| } |
There was a problem hiding this comment.
logic: potential race condition with multiple setTimeouts - consider using a ref to track and clear previous timeout
There was a problem hiding this comment.
I applied the code you suggested.
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| headers: headers as any, |
There was a problem hiding this comment.
style: Unsafe type assertion to 'any'. Consider defining proper types for the headers object.
There was a problem hiding this comment.
I don't have a good idea right now, so I'll improve it next time.
|
Hi @1bookmark-bot👋 Thanks for your contribution 🔥 Can you check greptile feedback? |
|
@pernielsentikaer I checked all the reviews. Thank you always!😄 |
pernielsentikaer
left a comment
There was a problem hiding this comment.
Hi 👋
Looks good to me, approved 🔥
|
Published to the Raycast Store: |
|
🎉 🎉 🎉 Such a great contribution deserves a reward, but unfortunately we couldn't find your Raycast account based on your GitHub username (@1bookmark-bot). |
Description
jotaiwhich causes unnecessary re-renders in raycast environment.Screencast
This is not a UI change.
Checklist
npm run buildand tested this distribution build in Raycastassetsfolder are used by the extension itselfREADMEare placed outside of themetadatafolder