Conversation
… ui-fixes Merge into main
… ui-fixes Merge into main
… ui-fixes Merge into main
… ui-fixes Merge into main
… ui-fixes Merge into main
… ui-fixes Merge into main
|
@Benjtalkshow is attempting to deploy a commit to the Threadflow Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughRemoved a redundant local useEffect in the notifications page, extended useNotifications with an enabled option and global-store sync, integrated notifications/auth into sidebar and user menu, and added auto‑validation UI for family wallet destination input. Changes
Sequence DiagramsequenceDiagram
participant Client as Client UI
participant Hook as useNotifications (polling/hook)
participant Store as useNotificationStore (global)
participant Sidebar as AppSidebar
participant NavUser as NavUser
Client->>Hook: hook polls / receives updates
activate Hook
Hook->>Store: setGlobalUnreadCount(unreadCount)
deactivate Hook
Store-->>Sidebar: unread count update
Store-->>NavUser: unread count update
Sidebar->>Sidebar: render badge if unread > 0
NavUser->>NavUser: render notification item badge
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
components/wallet/FamilyWalletDrawer.tsx (1)
234-256: Consider guarding against stale validation results.The auto-validation effect correctly debounces input but doesn't cancel in-flight API requests. If the user modifies the destination while
validateSendDestinationis pending, the stale response may briefly update the UI with incorrect results before being overwritten by the next validation.This is a minor UX concern—the state self-corrects—but for a cleaner experience, consider tracking the current destination at validation time and discarding results if it no longer matches.
♻️ Optional fix using a stale-check pattern
// Auto-validate destination address useEffect(() => { const trimmedDest = sendDestination.trim(); + let isStale = false; // Reset state if empty if (!trimmedDest) { setValidateResult('idle'); setValidateError(''); return; } + const runValidation = async () => { + await handleValidateDestination(); + }; + // Immediate trigger if 56 chars if (trimmedDest.length === 56) { - handleValidateDestination(); + runValidation(); return; } const timer = setTimeout(() => { - handleValidateDestination(); + if (!isStale) { + runValidation(); + } }, 500); - return () => clearTimeout(timer); + return () => { + isStale = true; + clearTimeout(timer); + }; }, [sendDestination, sendCurrency, handleValidateDestination]);For full protection against in-flight requests, you'd need to move the validation logic into this effect and use an
AbortControlleror check staleness afterawait.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/wallet/FamilyWalletDrawer.tsx` around lines 234 - 256, The auto-validate useEffect can let an in-flight validateSendDestination result overwrite state after the user has changed sendDestination; update the validation flow so each validation is tied to the destination value and ignored if stale: inside the effect (or inside handleValidateDestination) capture the current sendDestination (e.g., currentDest) or create an AbortController/token before calling validateSendDestination, and after the async call return check whether the token/currentDest still matches the latest sendDestination (or that the controller hasn’t been aborted) before calling setValidateResult or setValidateError; ensure to abort/ignore previous validations when sendDestination changes so stale responses don’t update state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/app-sidebar.tsx`:
- Around line 140-145: The notifications hook is being called before session
resolution because authClient.useSession() can return undefined initially,
causing useNotifications(userId || undefined) to open a socket without an
authenticated user; update the component to call useNotifications only when
session?.user?.id exists (or pass an enabled/autoConnect flag) so the hook
(useNotifications / underlying useSocket) does not auto-connect or sync unread
counts until userId is present; modify the call site (where userId is derived
from authClient.useSession) to guard or pass an explicit enabled/autoConnect
parameter to prevent early socket startup.
In `@components/nav-user.tsx`:
- Around line 43-44: Update the logout flow to clear the notification store
after sign-out: call the notification reset action from useNotificationStore
(e.g., reset or clearUnread) immediately after useAuthActions().logout() so
unreadNotifications is cleared before redirect; locate the logout invocation in
the component (the handler that calls logout) and add the notification reset
there. Also rename any event handler functions in this file (including the
logout handler and the handlers referenced around lines 137-140) to use the
"handle" prefix (e.g., handleLogout, handleMenuToggle) to follow the
event-handler naming guideline.
In `@hooks/useNotifications.ts`:
- Around line 50-55: The effect in useNotifications unconditionally writes the
local unreadCount (default 0) into the global useNotificationStore via
setGlobalUnreadCount on mount, which can overwrite authoritative data; change it
so this instance only updates the global store when it has authoritative data
(e.g., guard the useEffect with an "isLoaded" or "hasFetched" flag, or only sync
when unreadCount is non-null/undefined and not the default 0), or restrict
writes to a single owner by adding an "isOwner" prop/context; update the effect
that references setGlobalUnreadCount and unreadCount to check that authority
flag before calling setGlobalUnreadCount.
---
Nitpick comments:
In `@components/wallet/FamilyWalletDrawer.tsx`:
- Around line 234-256: The auto-validate useEffect can let an in-flight
validateSendDestination result overwrite state after the user has changed
sendDestination; update the validation flow so each validation is tied to the
destination value and ignored if stale: inside the effect (or inside
handleValidateDestination) capture the current sendDestination (e.g.,
currentDest) or create an AbortController/token before calling
validateSendDestination, and after the async call return check whether the
token/currentDest still matches the latest sendDestination (or that the
controller hasn’t been aborted) before calling setValidateResult or
setValidateError; ensure to abort/ignore previous validations when
sendDestination changes so stale responses don’t update state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0e40684f-40b2-44ce-bf90-8ab778124e3a
📒 Files selected for processing (5)
app/me/notifications/page.tsxcomponents/app-sidebar.tsxcomponents/nav-user.tsxcomponents/wallet/FamilyWalletDrawer.tsxhooks/useNotifications.ts
💤 Files with no reviewable changes (1)
- app/me/notifications/page.tsx
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/wallet/FamilyWalletDrawer.tsx (1)
214-240:⚠️ Potential issue | 🟠 MajorTie validation updates to the latest destination+asset pair.
validateSendDestination()is parameterized by bothdestinationPublicKeyandcurrency, but the stale-response checks here only comparesendDestination. If the user switches assets while a request is in flight, an old result can still mark the new asset as valid/invalid. The same guard can also strandvalidateLoading = true: once a request is superseded, itsfinallyskips the reset, and the replacement path may short-circuit before clearing it. Please track a request id/key that is invalidated on every destination/currency change and only commit or clear validation state for the latest attempt.Also applies to: 245-266
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/wallet/FamilyWalletDrawer.tsx` around lines 214 - 240, The validation result and loading clears must be tied to the latest destination+currency pair: introduce a mutable request key (e.g., validationKey) that you increment or set to a unique token whenever sendDestination or currency changes, pass/capture that key inside the async validation block around validateSendDestination, and before calling setValidateResult, setValidateError, setValidateErrorDetails or setValidateLoading ensure the captured key matches the current validationKey; update both the success branch and the catch/finally branches (the blocks using validateSendDestination, sendDestination, currency, setValidateResult, setValidateError, setValidateErrorDetails, setValidateLoading) so only the latest request can commit state or clear loading.
♻️ Duplicate comments (3)
hooks/useNotifications.ts (2)
60-65:⚠️ Potential issue | 🟠 MajorDon't publish
unreadCountafter a list fetch.
getNotifications()only updatesnotificationsandtotalhere. Once Line 100 flipshasFetched, the effect on Lines 61-65 pushes the still-local defaultunreadCount = 0intouseNotificationStore, andlib/stores/notification-store.tsoverwrites the shared badge value unconditionally. A newly mounted consumer can still clear a correct unread badge before the socket's'unread-count'event arrives. Gate the store sync on an authoritative unread-count source instead, or populateunreadCountfrom the fetch response before marking it ready.Also applies to: 100-100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/useNotifications.ts` around lines 60 - 65, The effect that syncs unreadCount into the shared store (useEffect watching unreadCount, setGlobalUnreadCount, hasFetched) runs after hasFetched flips and currently publishes a stale local unreadCount (default 0); modify the logic so you only call setGlobalUnreadCount when you have an authoritative unread count: either populate unreadCount from the getNotifications() response before setting hasFetched, or change the effect to gate on a new flag/field (e.g., authoritativeUnread or fetchedUnreadCount) that getNotifications sets from the fetch response; update references to unreadCount/setGlobalUnreadCount and getNotifications/hasFetched to use that authoritative value so the store is not overwritten by the default 0.
113-117:⚠️ Potential issue | 🟠 Major
enabledstill leaves the initial HTTP fetch active.The new caller passes
useNotifications({ enabled: !!userId }), but this effect still runs wheneverautoFetchis true. That means the sidebar can hit the notifications endpoint before session resolution even though the socket stays disconnected. Please gate the initial fetch withenabledtoo, and clearloadingwhen the hook is intentionally disabled.Suggested fix
useEffect(() => { - if (autoFetch) { - fetchNotifications(); - } - }, [fetchNotifications, autoFetch]); + if (!enabled || !autoFetch) { + setLoading(false); + return; + } + + fetchNotifications(); + }, [enabled, fetchNotifications, autoFetch]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/useNotifications.ts` around lines 113 - 117, The effect that triggers the initial HTTP fetch (useEffect in useNotifications) must be gated by the hook's enabled flag: update the effect to check both autoFetch and enabled before calling fetchNotifications and add enabled to the dependency array so it responds to changes; additionally, when enabled becomes false explicitly clear the loading state (e.g., setLoading(false)) so the hook does not remain stuck loading while intentionally disabled. Reference the useEffect, autoFetch, enabled, fetchNotifications and the loading state in your changes.components/nav-user.tsx (1)
47-50:⚠️ Potential issue | 🟠 MajorOnly clear unread state after sign-out succeeds.
logout()is async here. Kicking it off on Line 48 and clearing the store on Line 49 means a rejected sign-out leaves the user logged in with their unread badge already wiped, and the promise rejection is never awaited. Await the logout and clear the store only on success.Suggested fix
- const handleLogout = () => { - logout(); - clearUnreadCount(); - }; + const handleLogout = async (): Promise<void> => { + await logout(); + clearUnreadCount(); + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/nav-user.tsx` around lines 47 - 50, handleLogout currently calls logout() without awaiting it and immediately calls clearUnreadCount(), which can wipe unread state if sign-out fails; change handleLogout to await the async logout() call (or use .then/.catch) and only call clearUnreadCount() after logout resolves successfully, and handle/rethrow or log errors on rejected logout to avoid swallowing the rejection; refer to handleLogout, logout, and clearUnreadCount to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@components/wallet/FamilyWalletDrawer.tsx`:
- Around line 214-240: The validation result and loading clears must be tied to
the latest destination+currency pair: introduce a mutable request key (e.g.,
validationKey) that you increment or set to a unique token whenever
sendDestination or currency changes, pass/capture that key inside the async
validation block around validateSendDestination, and before calling
setValidateResult, setValidateError, setValidateErrorDetails or
setValidateLoading ensure the captured key matches the current validationKey;
update both the success branch and the catch/finally branches (the blocks using
validateSendDestination, sendDestination, currency, setValidateResult,
setValidateError, setValidateErrorDetails, setValidateLoading) so only the
latest request can commit state or clear loading.
---
Duplicate comments:
In `@components/nav-user.tsx`:
- Around line 47-50: handleLogout currently calls logout() without awaiting it
and immediately calls clearUnreadCount(), which can wipe unread state if
sign-out fails; change handleLogout to await the async logout() call (or use
.then/.catch) and only call clearUnreadCount() after logout resolves
successfully, and handle/rethrow or log errors on rejected logout to avoid
swallowing the rejection; refer to handleLogout, logout, and clearUnreadCount to
locate the change.
In `@hooks/useNotifications.ts`:
- Around line 60-65: The effect that syncs unreadCount into the shared store
(useEffect watching unreadCount, setGlobalUnreadCount, hasFetched) runs after
hasFetched flips and currently publishes a stale local unreadCount (default 0);
modify the logic so you only call setGlobalUnreadCount when you have an
authoritative unread count: either populate unreadCount from the
getNotifications() response before setting hasFetched, or change the effect to
gate on a new flag/field (e.g., authoritativeUnread or fetchedUnreadCount) that
getNotifications sets from the fetch response; update references to
unreadCount/setGlobalUnreadCount and getNotifications/hasFetched to use that
authoritative value so the store is not overwritten by the default 0.
- Around line 113-117: The effect that triggers the initial HTTP fetch
(useEffect in useNotifications) must be gated by the hook's enabled flag: update
the effect to check both autoFetch and enabled before calling fetchNotifications
and add enabled to the dependency array so it responds to changes; additionally,
when enabled becomes false explicitly clear the loading state (e.g.,
setLoading(false)) so the hook does not remain stuck loading while intentionally
disabled. Reference the useEffect, autoFetch, enabled, fetchNotifications and
the loading state in your changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 93788055-a217-40d1-951d-5ecdd737ea0d
📒 Files selected for processing (4)
components/app-sidebar.tsxcomponents/nav-user.tsxcomponents/wallet/FamilyWalletDrawer.tsxhooks/useNotifications.ts
1 participant
Summary by CodeRabbit
New Features
Refactor