Conversation
WalkthroughPrefetch condition changed to run when no cache or cache older than 60s; query-key listener now observes Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Page as Notifications Page
participant Cache as Query Cache
participant API as Server/API
participant RQW as ReactQueryWrapper
participant Hook as useNotificationsQuery
Note over Page,Cache: User opens notifications page
User->>Page: Navigate to notifications
Page->>Cache: Check notifications cache + timestamp
alt No cache OR stale (>60s)
Page->>API: Prefetch notifications
API-->>Cache: Store fresh notifications
else Fresh cache
Page->>Cache: Use cached data
end
Note over Cache,RQW: Query key update observed
Cache-->>RQW: IDENTITY_NOTIFICATIONS updated
RQW->>RQW: Cookies.set(timestamp for IDENTITY_NOTIFICATIONS)
Note over Hook,Cache: Hook derives view model
Hook->>Cache: Read query result
Hook->>Hook: items = memoized(flatten(query.data))
Hook->>Hook: isInitialQueryDone = isSuccess OR isError
Hook-->>Page: Return { ...query, items, isInitialQueryDone }
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (2)**/*.{ts,tsx}📄 CodeRabbit inference engine (.cursorrules)
Files:
**/*.tsx📄 CodeRabbit inference engine (.cursorrules)
Files:
🧬 Code graph analysis (1)hooks/useNotificationsQuery.tsx (1)
🔇 Additional comments (4)
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: 0
🧹 Nitpick comments (1)
hooks/useNotificationsQuery.tsx (1)
88-96: Wrap client-side prefetch in useEffect
The call toqueryClient.prefetchInfiniteQueryinsideuseNotificationsQueryruns on every render—move it into auseEffectwith[identity, limit, cause]so it only fires when those change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/my-stream/notifications/page.tsx(1 hunks)components/react-query-wrapper/ReactQueryWrapper.tsx(1 hunks)hooks/useNotificationsQuery.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{ts,tsx}: Do not include any comments in the code
Use react-query for data fetching
Always add readonly before propsFollow existing code style and naming conventions
Files:
components/react-query-wrapper/ReactQueryWrapper.tsxapp/my-stream/notifications/page.tsxhooks/useNotificationsQuery.tsx
**/*.tsx
📄 CodeRabbit inference engine (.cursorrules)
**/*.tsx: Use FontAwesome for icons
Use TailwindCSS for stylingUse React functional components with hooks for UI components
Files:
components/react-query-wrapper/ReactQueryWrapper.tsxapp/my-stream/notifications/page.tsxhooks/useNotificationsQuery.tsx
{app,pages}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
Use NextJS features that match the current version
Files:
app/my-stream/notifications/page.tsx
app/**
📄 CodeRabbit inference engine (AGENTS.md)
All new Next.js pages must be created under the
app/directory
Files:
app/my-stream/notifications/page.tsx
app/**/{page,layout}.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Routes in
app/should export agenerateMetadatafunction usinggetAppMetadata
Files:
app/my-stream/notifications/page.tsx
🧬 Code graph analysis (2)
components/react-query-wrapper/ReactQueryWrapper.tsx (1)
hooks/useQueryKeyListener.ts (1)
useQueryKeyListener(6-27)
hooks/useNotificationsQuery.tsx (1)
types/feed.types.ts (2)
TypedNotification(135-143)TypedNotificationsResponse(145-148)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (6)
hooks/useNotificationsQuery.tsx (3)
3-3: LGTM!The
useMemoimport is correctly added to support the new memoized items derivation below.
111-121: LGTM! Clean refactor to derived state.Replacing useState with useMemo eliminates the synchronization complexity of useEffect and ensures items are always derived from the current query data. The reverse logic correctly creates a new array when needed.
123-123: Confirm disabled-query readiness
TheisInitialQueryDoneflag is false whenenabled: false(e.g., noidentityoractiveProfileProxyis true). Verify that downstream components (e.g. Notifications) handle loader vs. no-items rendering correctly in this scenario.app/my-stream/notifications/page.tsx (1)
23-26: LGTM! Fixes first-load flickering.The updated condition now prefetches when there's no cached value (
!notificationsFetched) or when stale, addressing the first-load flickering mentioned in the PR title. Previously, prefetch only occurred if a cached value existed and was stale, missing the initial-load case.components/react-query-wrapper/ReactQueryWrapper.tsx (2)
1197-1202: LGTM! Correctly tracks IDENTITY_NOTIFICATIONS.The listener now tracks
IDENTITY_NOTIFICATIONSquery updates and sets the corresponding cookie timestamp, which is read inpage.tsx(line 19-21) to determine when to prefetch. The implementation is consistent with the existing pattern.
1193-1195: Remove or confirm necessity of FEED_ITEMS listenerNo reads of the
FEED_ITEMScookie were found beyond thisCookies.setcall. If this listener is no longer needed, remove it; otherwise, clarify why it must remain.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
__tests__/hooks/useNotificationsQuery.test.tsx (1)
39-43: Consider adding an error state test case.While this test correctly verifies the in-progress state (
isSuccess: false, isError: false), consider adding a test case whereisError: trueto verify thatisInitialQueryDonebecomestrueeven when the query fails.Example test case:
it('marks initial query as done on error', () => { useInfiniteQueryMock.mockReturnValue({ data: undefined, isSuccess: false, isError: true, error: new Error('Network error') }); const { result } = renderHook(() => useNotificationsQuery({ identity: 'id' })); expect(result.current.items).toEqual([]); expect(result.current.isInitialQueryDone).toBe(true); });hooks/useNotificationsQuery.tsx (2)
88-102: Consider reducing duplication withusePrefetchNotifications.The prefetch configuration here duplicates the logic in
usePrefetchNotifications(lines 154-162). Consider refactoring to reuse the prefetch function:const queryClient = useQueryClient(); + const prefetch = usePrefetchNotifications(); useEffect(() => { if (!identity || activeProfileProxy) { return; } - queryClient.prefetchInfiniteQuery({ - queryKey: getIdentityNotificationsQueryKey(identity, limit, cause), - queryFn: ({ pageParam }: { pageParam?: number | null }) => - fetchNotifications({ limit, cause, pageParam }), - initialPageParam: null, - getNextPageParam: (lastPage) => lastPage.notifications.at(-1)?.id ?? null, - pages: 3, - staleTime: 60000, - }); + prefetch({ identity, limit, cause }); - }, [queryClient, identity, activeProfileProxy, limit, cause]); + }, [prefetch, identity, activeProfileProxy, limit, cause]);
117-127: LGTM! Optional: Simplify the type cast.The memoized derivation is correct and efficient. The empty array cast on line 119 is redundant since TypeScript can infer the type from the return type, but it doesn't hurt.
Optional simplification:
const items = useMemo(() => { if (!query.data) { - return [] as TypedNotification[]; + return []; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
__tests__/hooks/useNotificationsQuery.test.tsx(2 hunks)hooks/useNotificationsQuery.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{ts,tsx}: Do not include any comments in the code
Use react-query for data fetching
Always add readonly before propsFollow existing code style and naming conventions
Files:
hooks/useNotificationsQuery.tsx__tests__/hooks/useNotificationsQuery.test.tsx
**/*.tsx
📄 CodeRabbit inference engine (.cursorrules)
**/*.tsx: Use FontAwesome for icons
Use TailwindCSS for stylingUse React functional components with hooks for UI components
Files:
hooks/useNotificationsQuery.tsx__tests__/hooks/useNotificationsQuery.test.tsx
__tests__/**
📄 CodeRabbit inference engine (tests/AGENTS.md)
Place Jest test suites under the
__tests__directory mirroring source folders (e.g., components, contexts, hooks, utils)
Files:
__tests__/hooks/useNotificationsQuery.test.tsx
{**/__tests__/**,**/*.test.{ts,tsx}}
📄 CodeRabbit inference engine (AGENTS.md)
{**/__tests__/**,**/*.test.{ts,tsx}}: If coverage on a modified file is below 80%, add meaningful tests to raise it to at least 80%
Mock external dependencies and APIs in tests
Files:
__tests__/hooks/useNotificationsQuery.test.tsx
**/__tests__/**
📄 CodeRabbit inference engine (AGENTS.md)
Place tests in
__tests__/directories when organizing standalone test suites
Files:
__tests__/hooks/useNotificationsQuery.test.tsx
**/*.test.tsx
📄 CodeRabbit inference engine (AGENTS.md)
When co-locating tests with components, name them
ComponentName.test.tsx
Files:
__tests__/hooks/useNotificationsQuery.test.tsx
🧬 Code graph analysis (2)
hooks/useNotificationsQuery.tsx (2)
components/navigation/BottomNavigation.tsx (1)
items(16-68)types/feed.types.ts (2)
TypedNotification(135-143)TypedNotificationsResponse(145-148)
__tests__/hooks/useNotificationsQuery.test.tsx (1)
hooks/useNotificationsQuery.tsx (1)
useNotificationsQuery(75-137)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
__tests__/hooks/useNotificationsQuery.test.tsx (2)
28-30: LGTM!The mock correctly includes
isSuccessandisErrorflags that align with the updated implementation whereisInitialQueryDoneis derived fromquery.isSuccess || query.isError.
51-53: LGTM!The test correctly simulates identity change behavior where the new query starts in an incomplete state (
isSuccess: false, isError: false), causingisInitialQueryDoneto reset tofalseanditemsto reset to an empty array.Also applies to: 61-65
hooks/useNotificationsQuery.tsx (2)
3-3: LGTM!The
useMemoimport is necessary for the memoizeditemsderivation implemented below.
129-130: LGTM!The derived
isInitialQueryDonecorrectly identifies when the initial query completes (either with success or error). This pattern is cleaner than explicit state tracking and ensures the value stays in sync with the query state.
|
2 participants
Summary by CodeRabbit
Bug Fixes
Improvements