fixing jotai persistent state #904
Conversation
|
@ikemHood is attempting to deploy a commit to the LFG Labs Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe changes made in the pull request focus on the Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Hey @fricoben, how can i possibly test this fix? I have tried performing some quest to see if i can claim a reward and perform a transaction, but the events i tried did not allow me claim rewards immediately |
|
The previos implementation updates the array but not the reference of the array, which doesn't trigger jotai to update the store. |
|
Hey @revengexd, Please review. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
hooks/useNotificationManager.ts (1)
Line range hint
42-51: Add error handling for provider calls.The
checkTransactionStatusfunction should handle potential provider errors to prevent undefined behavior.Apply this improvement:
const checkTransactionStatus = async (txHash: string) => { + try { const data = await provider.getTransactionReceipt(txHash); if (data?.isRejected() || data?.isReverted()) { updateNotificationStatus(txHash, "error"); setUnread(true); } else if (data?.isSuccess()) { updateNotificationStatus(txHash, "success"); setUnread(true); } + } catch (error) { + console.error('Failed to check transaction status:', error); + updateNotificationStatus(txHash, "error"); + } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- hooks/useNotificationManager.ts (1 hunks)
🔇 Additional comments (1)
hooks/useNotificationManager.ts (1)
Line range hint
1-85: Verify the notification polling implementation.To properly diagnose the infinite loop issue, we need to verify how this hook is being used in the notification polling mechanism.
| @@ -35,7 +35,7 @@ export function useNotificationManager() { | |||
| return notification; | |||
| }); | |||
|
|
|||
| setNotifications(updatedNotifications); | |||
| setNotifications([...updatedNotifications]); | |||
There was a problem hiding this comment.
The spread operator change alone may not resolve the infinite loop.
While creating a new array reference can help with React's change detection, it doesn't address the root cause of the infinite loop. The issue might be related to how transaction status checks are triggered and managed.
Consider implementing these improvements:
- Add a completion flag to stop checking transaction status:
const checkTransactionStatus = async (txHash: string) => {
// Prevent re-checking completed transactions
const existingNotification = notifications.find(n => n.data.hash === txHash);
if (existingNotification?.data.status === 'success' ||
existingNotification?.data.status === 'error') {
return;
}
try {
const data = await provider.getTransactionReceipt(txHash);
if (data?.isRejected() || data?.isReverted()) {
updateNotificationStatus(txHash, "error");
setUnread(true);
} else if (data?.isSuccess()) {
updateNotificationStatus(txHash, "success");
setUnread(true);
}
} catch (error) {
console.error('Failed to check transaction status:', error);
updateNotificationStatus(txHash, "error");
}
};- If this hook is used in conjunction with a polling mechanism, implement proper cleanup:
useEffect(() => {
const intervals: NodeJS.Timeout[] = [];
notifications
.filter(n => n.data.status === 'pending')
.forEach(n => {
const interval = setInterval(() => {
checkTransactionStatus(n.data.hash);
}, 5000); // Poll every 5 seconds
intervals.push(interval);
});
return () => intervals.forEach(clearInterval);
}, [notifications]);There was a problem hiding this comment.
AI for code review 😂 😂
There was a problem hiding this comment.
@ikemHood 😊 I'm glad you found the AI code review helpful!
/)/)
( ^.^)
c(")(")
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Pull Request type
Please add the labels corresponding to the type of changes your PR introduces:
Resolves: #890
Other information
Summary by CodeRabbit