Conversation
Contributor
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThis is a multi-package release that fixes friends list data handling and pagination across the web app and SDK. The main change streamlines friends list filtering and pagination logic, updates the FriendsList component's mode prop to a discriminated union type, improves null-safety in profile update mutations, and bumps versions in SDK and wallets packages. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/web/src/app/(dynamicPages)/profile/[username]/_components/friends/friends-list.tsx (2)
29-37: CustomhasMorere-implementshasNextPagealready provided byuseInfiniteQuery.
getNextPageParamin the query options already encodes the "is there a next page" logic (lastPage.length === limit).useInfiniteQuerysurfaces this as ahasNextPageboolean. The customhasMorememo duplicates that logic and could silently diverge ifgetNextPageParamchanges.♻️ Proposed refactor — use built-in hasNextPage
const { data, isFetching: isFriendsFetching, - fetchNextPage + fetchNextPage, + hasNextPage } = useInfiniteQuery( getFriendsInfiniteQueryOptions(account.name, mode, { limit: loadLimit }) ); - const hasMore = useMemo( - () => (data?.pages?.[data.pages.length - 1]?.length ?? 0) >= loadLimit, - [data?.pages] - );Then replace
hasMorewithhasNextPageat the use site (line 149):- <Button disabled={isFetching || !hasMore} onClick={() => fetchNextPage()}> + <Button disabled={isFetching || !hasNextPage} onClick={() => fetchNextPage()}>Also applies to: 84-87
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/`(dynamicPages)/profile/[username]/_components/friends/friends-list.tsx around lines 29 - 37, The custom memo "hasMore" duplicates pagination logic already provided by useInfiniteQuery; update the component to stop computing hasMore and instead use the built-in hasNextPage returned by useInfiniteQuery (from the call that uses getFriendsInfiniteQueryOptions). Replace uses of the custom hasMore (and its related memo) with hasNextPage and keep calling fetchNextPage when you need to load more; ensure you remove or refactor the memoized hasMore logic so it cannot diverge from getNextPageParam.
56-81:dayjs(item.active)computed twice per item — consolidate into a single pass.
dayjs(item.active)is constructed in thefilter(line 59) and then again in themap(line 80). Combine into a single.reduce()or pre-compute to one iteration:♻️ Proposed refactor — single pass over rawItems
- const filtered = rawItems.filter((item) => { - if (!type) return true; - const lastSeenTime = dayjs(item.active).toDate(); - const timeDifference = new Date().getTime() - lastSeenTime.getTime(); - const daysDifference = Math.ceil(timeDifference / (1000 * 3600 * 24)); - const yearsDifference = Math.ceil(daysDifference / 365); - return ( - (type === FilterFriendsType.Recently && daysDifference < 7) || ... - ); - }); - - return filtered.map((item) => ({ - name: item.name, - reputation: item.reputation, - lastSeen: dayjs(item.active).fromNow() - })); + return rawItems.reduce<Friend[]>((acc, item) => { + const dayjsActive = dayjs(item.active); + if (type) { + const timeDifference = new Date().getTime() - dayjsActive.toDate().getTime(); + const daysDifference = Math.ceil(timeDifference / (1000 * 3600 * 24)); + const yearsDifference = Math.ceil(daysDifference / 365); + const matches = + (type === FilterFriendsType.Recently && daysDifference < 7) || ...; + if (!matches) return acc; + } + acc.push({ name: item.name, reputation: item.reputation, lastSeen: dayjsActive.fromNow() }); + return acc; + }, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/`(dynamicPages)/profile/[username]/_components/friends/friends-list.tsx around lines 56 - 81, The code is creating dayjs(item.active) twice for each rawItems entry; refactor the filter+map into a single pass (use Array.prototype.reduce or a single Array.prototype.map with conditional inclusion) over rawItems and compute const active = dayjs(item.active) once per item, then reuse active.toDate() / active.fromNow() and the computed daysDifference/yearsDifference to decide inclusion against FilterFriendsType and produce the output shape ({ name, reputation, lastSeen }), replacing the current filtered/map logic.apps/web/src/features/shared/fragments/fragments-list.tsx (1)
47-47: Defensive null-guard is correct; consider.includes()for readability.The
?? ""guard prevents aTypeErrorwhentitleisundefined. Optionally,.indexOf(...) !== -1can be replaced with the more idiomatic.includes():♻️ Optional readability refactor
- .filter((x) => (x.title ?? "").toLowerCase().indexOf(searchQuery.toLowerCase()) !== -1) + .filter((x) => (x.title ?? "").toLowerCase().includes(searchQuery.toLowerCase()))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/features/shared/fragments/fragments-list.tsx` at line 47, The filter callback currently uses (x.title ?? "").toLowerCase().indexOf(searchQuery.toLowerCase()) !== -1 which is defensive but less readable; update the predicate in the fragments list filter to keep the null-guard (x.title ?? "") and replace indexOf(...) !== -1 with .includes(...) on the lowercased string so it reads (x.title ?? "").toLowerCase().includes(searchQuery.toLowerCase()), ensuring you still call toLowerCase() on both x.title and searchQuery in the same filter function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/web/src/app/`(dynamicPages)/profile/[username]/_components/friends/friends-list.tsx:
- Around line 64-74: The filter predicates in friends-list.tsx (the block using
FilterFriendsType) have exclusive boundaries and an exact-match for OneYear that
create gaps; change them to use inclusive ranges and make OneYear a range
instead of daysDifference === 365. Specifically: use daysDifference <= 7 for
Recently; use daysDifference > 7 && daysDifference <= 30 for ThisMonth; use
daysDifference > 30 && daysDifference < 365 for ThisYear (or <= 364 if you
prefer 365 as the start of OneYear); make OneYear a range like daysDifference >=
365 && daysDifference < 730 (one-to-two years) instead of an exact equality; and
make MoreThanOneYear match yearsDifference >= 2 (or yearsDifference >= 1 if you
want >1 year inclusive) so there are no gaps between these predicates.
---
Nitpick comments:
In
`@apps/web/src/app/`(dynamicPages)/profile/[username]/_components/friends/friends-list.tsx:
- Around line 29-37: The custom memo "hasMore" duplicates pagination logic
already provided by useInfiniteQuery; update the component to stop computing
hasMore and instead use the built-in hasNextPage returned by useInfiniteQuery
(from the call that uses getFriendsInfiniteQueryOptions). Replace uses of the
custom hasMore (and its related memo) with hasNextPage and keep calling
fetchNextPage when you need to load more; ensure you remove or refactor the
memoized hasMore logic so it cannot diverge from getNextPageParam.
- Around line 56-81: The code is creating dayjs(item.active) twice for each
rawItems entry; refactor the filter+map into a single pass (use
Array.prototype.reduce or a single Array.prototype.map with conditional
inclusion) over rawItems and compute const active = dayjs(item.active) once per
item, then reuse active.toDate() / active.fromNow() and the computed
daysDifference/yearsDifference to decide inclusion against FilterFriendsType and
produce the output shape ({ name, reputation, lastSeen }), replacing the current
filtered/map logic.
In `@apps/web/src/features/shared/fragments/fragments-list.tsx`:
- Line 47: The filter callback currently uses (x.title ??
"").toLowerCase().indexOf(searchQuery.toLowerCase()) !== -1 which is defensive
but less readable; update the predicate in the fragments list filter to keep the
null-guard (x.title ?? "") and replace indexOf(...) !== -1 with .includes(...)
on the lowercased string so it reads (x.title ??
"").toLowerCase().includes(searchQuery.toLowerCase()), ensuring you still call
toLowerCase() on both x.title and searchQuery in the same filter function.
apps/web/src/app/(dynamicPages)/profile/[username]/_components/friends/friends-list.tsx
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/web/src/api/mutations/update-profile.ts (1)
42-44:onErrorconflates null-account errors with server errorsThe null guard on lines 42-44 throws
"Account is not available"butonErrorunconditionally shows"g.server-error", swallowing the distinction. A user who hits this path (account not yet loaded) will see a misleading server-error toast.♻️ Proposed fix
- onError: () => { - error(i18next.t("g.server-error")); + onError: (err: Error) => { + error(err.message === "Account is not available" + ? err.message + : i18next.t("g.server-error")); },Or, preferably, disable the mutate call at the call site when
accountis null (e.g. guard the caller) so this code path is never reached at all.Also applies to: 52-57
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/api/mutations/update-profile.ts` around lines 42 - 44, The guard that currently does "if (!account) throw new Error('Account is not available')" conflates a missing-account situation with server errors in the shared onError handler; either prevent calling the mutate when account is null at the caller, or change this check to surface a distinguishable condition (e.g., return early or throw a specific error type/value like AccountUnavailableError or an Error with a machine-readable code) so onError can detect and handle it separately; apply the same change to the similar null-account guard at the other block (lines 52-57) and update onError to look for that specific error type/code (or rely on the caller-side guard) rather than treating every error as "g.server-error".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/api/mutations/pin-to-blog.ts`:
- Line 15: The TypeScript error occurs because account (from useActiveAccount)
can be undefined but useUpdateProfile expects FullAccount | null; update the
call that creates updateProfile (the line with const { mutateAsync:
updateProfile } = useUpdateProfile(account);) to pass a value of type
FullAccount | null by converting undefined to null (e.g., pass account ?? null)
so useUpdateProfile receives either a FullAccount or null and the compile error
is resolved.
In
`@apps/web/src/app/`(dynamicPages)/profile/[username]/_components/friends/friends-list.tsx:
- Around line 60-62: Guard against falsy item.active before converting with
dayjs: if item.active is undefined/null/empty, skip computing
lastSeenTime/timeDifference/daysDifference (or set a sentinel like null) so
those users aren’t treated as "Recently"; update the logic around lastSeenTime,
timeDifference and daysDifference in friends-list.tsx to early-return or branch
when item.active is falsy and ensure downstream rendering/grouping ignores or
places such items in the appropriate bucket instead of treating them as now.
---
Nitpick comments:
In `@apps/web/src/api/mutations/update-profile.ts`:
- Around line 42-44: The guard that currently does "if (!account) throw new
Error('Account is not available')" conflates a missing-account situation with
server errors in the shared onError handler; either prevent calling the mutate
when account is null at the caller, or change this check to surface a
distinguishable condition (e.g., return early or throw a specific error
type/value like AccountUnavailableError or an Error with a machine-readable
code) so onError can detect and handle it separately; apply the same change to
the similar null-account guard at the other block (lines 52-57) and update
onError to look for that specific error type/code (or rely on the caller-side
guard) rather than treating every error as "g.server-error".
apps/web/src/app/(dynamicPages)/profile/[username]/_components/friends/friends-list.tsx
Show resolved
Hide resolved
feruzm
added
the
patch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
Bug Fixes
Chores