Conversation
WalkthroughRename NotificationsWrapper prop Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Scroll as Notifications scroll container
participant Store as DataFetcher
participant UI as NotificationsWrapper/Items
User->>Scroll: onScroll(event)
Note over Scroll: track refs (isPrepending, prevScrollHeight), observed items
alt Near top (<= NEAR_TOP_SCROLL_THRESHOLD_PX)
Scroll->>Store: fetchOlderPage()
Store-->>Scroll: older items
Scroll->>Scroll: set isPrepending & preserve scrollTop
Scroll->>UI: render with loadingOlder
else Scrolling up (not near top)
Scroll->>Scroll: IntersectionObserver updates out-of-view count
alt out-of-view <= MIN_OUT_OF_VIEW_COUNT
Scroll->>Store: fetchOlderPage()
end
end
opt Scrolling down near bottom
Scroll->>Store: fetchNextPage()
end
Note right of Scroll: MutationObserver keeps observations up-to-date
sequenceDiagram
autonumber
participant C as FeedScrollContainer (refactored)
participant Up as onScrollUpNearTop
participant Down as onScrollDownNearBottom
C->>C: handle onScroll(event)
alt Near top (<= NEAR_TOP_SCROLL_THRESHOLD_PX)
C->>Up: invoke onScrollUpNearTop()
Note right of C: clear throttle and return
else Scrolling up
C->>C: observe feed items (MutationObserver)
C->>C: count out-of-view items (IntersectionObserver)
alt out-of-view <= MIN_OUT_OF_VIEW_COUNT
C->>Up: invoke onScrollUpNearTop()
end
end
opt Scrolling down near bottom
C->>Down: invoke onScrollDownNearBottom()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/brain/notifications/Notifications.tsx (1)
51-65: Fix temporal dead zone: move query/mutation declarations before this effect.This effect references
refetchandmarkAllAsReadMutationbefore those consts are initialized. On the first render this throwsReferenceError: Cannot access 'refetch' before initialization, so the component never mounts. Please reorder so theuseNotificationsQuerydestructuring (and themarkAllAsReadMutationsetup) come before this effect. One option:- useEffect(() => { - if (reload === "true") { - refetch() - .then(() => { - return markAllAsReadMutation.mutateAsync(); - }) - .catch((error) => { - console.error("Error during refetch:", error); - }); - const params = new URLSearchParams(searchParams?.toString() || ''); - params.delete('reload'); - const newUrl = params.toString() ? `${pathname}?${params.toString()}` : (pathname || '/my-stream/notifications'); - router.replace(newUrl, { scroll: false }); - } - }, [reload]); - - const { invalidateNotifications } = useContext(ReactQueryWrapperContext); - - const markAllAsReadMutation = useMutation({ + const { invalidateNotifications } = useContext(ReactQueryWrapperContext); + + const markAllAsReadMutation = useMutation({ … }); + const { + items, + isFetching, + isFetchingNextPage, + hasNextPage, + fetchNextPage, + refetch, + isInitialQueryDone, + } = useNotificationsQuery({ … }); + + useEffect(() => { + if (reload === "true") { + refetch() + .then(() => markAllAsReadMutation.mutateAsync()) + .catch((error) => { + console.error("Error during refetch:", error); + }); + const params = new URLSearchParams(searchParams?.toString() || ''); + params.delete('reload'); + const newUrl = params.toString() + ? `${pathname}?${params.toString()}` + : pathname || '/my-stream/notifications'; + router.replace(newUrl, { scroll: false }); + } + }, [reload]);Adjust the dependency list if you hoist new references.
🧹 Nitpick comments (1)
components/brain/notifications/NotificationItem.tsx (1)
104-119: Memoization applied correctly; custom comparison is redundant.The memoization implementation is correct and will prevent unnecessary re-renders when props haven't changed. The callbacks are stable (wrapped in
useCallbackin NotificationsWrapper), so the reference equality checks will work as intended.However, the custom comparison function performs a shallow equality check on all props, which is exactly what
memodoes by default when no comparison function is provided. You can simplify this by removing the custom comparison function.Apply this diff to use the default memo behavior:
-const NotificationItem = memo( - NotificationItemComponent, - (prevProps, nextProps) => { - return ( - prevProps.notification === nextProps.notification && - prevProps.activeDrop === nextProps.activeDrop && - prevProps.onReply === nextProps.onReply && - prevProps.onQuote === nextProps.onQuote && - prevProps.onDropContentClick === nextProps.onDropContentClick - ); - } -); +const NotificationItem = memo(NotificationItemComponent);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
__tests__/components/brain/NotificationsWrapper.test.tsx(1 hunks)__tests__/components/brain/notifications/NotificationItems.test.tsx(0 hunks)__tests__/components/brain/notifications/Notifications.test.tsx(3 hunks)components/brain/feed/FeedScrollContainer.tsx(2 hunks)components/brain/notifications/NotificationItem.tsx(3 hunks)components/brain/notifications/NotificationItems.tsx(2 hunks)components/brain/notifications/Notifications.tsx(5 hunks)components/brain/notifications/NotificationsWrapper.tsx(2 hunks)
💤 Files with no reviewable changes (1)
- tests/components/brain/notifications/NotificationItems.test.tsx
🧰 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 propsUse TypeScript for implementation code
Files:
__tests__/components/brain/notifications/Notifications.test.tsxcomponents/brain/feed/FeedScrollContainer.tsx__tests__/components/brain/NotificationsWrapper.test.tsxcomponents/brain/notifications/NotificationItems.tsxcomponents/brain/notifications/NotificationsWrapper.tsxcomponents/brain/notifications/NotificationItem.tsxcomponents/brain/notifications/Notifications.tsx
**/*.tsx
📄 CodeRabbit inference engine (.cursorrules)
**/*.tsx: Use FontAwesome for icons
Use TailwindCSS for stylingUse React functional components with hooks
Files:
__tests__/components/brain/notifications/Notifications.test.tsxcomponents/brain/feed/FeedScrollContainer.tsx__tests__/components/brain/NotificationsWrapper.test.tsxcomponents/brain/notifications/NotificationItems.tsxcomponents/brain/notifications/NotificationsWrapper.tsxcomponents/brain/notifications/NotificationItem.tsxcomponents/brain/notifications/Notifications.tsx
**/{__tests__/**/*.{ts,tsx},*.test.tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/{__tests__/**/*.{ts,tsx},*.test.tsx}: Place tests in tests directories or alongside components as ComponentName.test.tsx
Mock external dependencies and APIs in tests
Files:
__tests__/components/brain/notifications/Notifications.test.tsx__tests__/components/brain/NotificationsWrapper.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__/components/brain/notifications/Notifications.test.tsx__tests__/components/brain/NotificationsWrapper.test.tsx
__tests__/components/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (tests/AGENTS.md)
Use
@testing-library/reactand@testing-library/user-eventfor React component tests
Files:
__tests__/components/brain/notifications/Notifications.test.tsx__tests__/components/brain/NotificationsWrapper.test.tsx
🧬 Code graph analysis (4)
__tests__/components/brain/notifications/Notifications.test.tsx (1)
components/brain/notifications/Notifications.tsx (1)
Notifications(29-221)
__tests__/components/brain/NotificationsWrapper.test.tsx (1)
components/brain/notifications/NotificationsWrapper.tsx (1)
NotificationsWrapper(21-79)
components/brain/notifications/NotificationsWrapper.tsx (3)
types/dropInteractionTypes.ts (1)
ActiveDropState(8-12)helpers/waves/drop.helpers.ts (1)
ExtendedDrop(16-20)components/waves/drops/Drop.tsx (1)
DropInteractionParams(13-16)
components/brain/notifications/Notifications.tsx (3)
components/common/SpinnerLoader.tsx (1)
SpinnerLoader(8-57)components/brain/my-stream/layout/MyStreamNoItems.tsx (1)
MyStreamNoItems(4-126)components/brain/notifications/NotificationsWrapper.tsx (1)
NotificationsWrapper(21-79)
⏰ 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 (3)
__tests__/components/brain/NotificationsWrapper.test.tsx (2)
26-37: LGTM! Test updated correctly for the new prop name.The test properly reflects the prop rename from
loadingtoloadingOlder, and the text assertion matches the new loading message "Loading older notifications..." rendered by NotificationsWrapper.
39-52: LGTM! Callback delegation test properly updated.The test correctly uses
loadingOlder={false}and validates that the wrapper properly delegates callbacks to the router and state setter.components/brain/notifications/NotificationItem.tsx (1)
1-1: LGTM! Memo import and component rename set up memoization correctly.The import of
memoand rename toNotificationItemComponentprepare the component for proper memoization.Also applies to: 18-18
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
components/brain/notifications/NotificationItems.tsx (1)
23-30: Use a stable, non-index fallback for keys.Index fallback still remounts items on prepend when ids are missing. Attach a stable client key at data-build time (e.g., derived hash or stored
clientKey) and use that instead of position.
🧹 Nitpick comments (8)
components/brain/feed/FeedScrollContainer.tsx (2)
115-134: Performance: querySelectorAll + getBoundingClientRect loop can be expensive.On large feeds, calling
querySelectorAllfollowed bygetBoundingClientRectfor each element in a loop (even throttled) can cause jank. Consider using Intersection Observer API for more efficient visibility tracking.Replace the element visibility loop with Intersection Observer:
useEffect(() => { if (!contentRef.current || !ref || !("current" in ref) || !ref.current) return; const observer = new IntersectionObserver( (entries) => { const outOfViewCount = entries.filter(entry => !entry.isIntersecting && entry.boundingClientRect.bottom < 0).length; if (outOfViewCount <= MIN_OUT_OF_VIEW_COUNT) { onScrollUpNearTop(); } }, { root: ref.current, threshold: 0 } ); const elements = contentRef.current.querySelectorAll("[id^='feed-item-']"); elements.forEach(el => observer.observe(el)); return () => observer.disconnect(); }, [onScrollUpNearTop]);Note: This would require refactoring the scroll handling logic to coordinate with the observer.
78-145: Suggest extracting scroll logic into smaller functions.The
handleScrollcallback is complex (65+ lines with nested conditionals). Extract the near-top, downward, and upward scroll logic into separate functions to improve readability and testability.Example structure:
const handleNearTopScroll = useCallback(() => { onScrollUpNearTop(); }, [onScrollUpNearTop]); const handleDownwardScroll = useCallback((currentTarget: HTMLDivElement) => { if (!onScrollDownNearBottom) return false; const { scrollHeight, scrollTop, clientHeight } = currentTarget; const scrolledToBottom = scrollHeight - scrollTop - clientHeight < 100; if (scrolledToBottom) { onScrollDownNearBottom(); return true; } return false; }, [onScrollDownNearBottom]); const handleUpwardScrollWithVisibility = useCallback((currentTarget: HTMLDivElement) => { // ... existing element visibility logic }, [onScrollUpNearTop]);components/brain/notifications/Notifications.tsx (6)
22-22: Use a single source of truth for scroll threshold.160px here vs 200px in FeedScrollContainer. Export one constant from a shared module and reuse to avoid drift.
86-88: Avoid double “mark all as read” when?reload=true.Called here and again inside the reload flow. Guard with
reloadto run once.-useEffect(() => { - markAllAsReadMutation.mutateAsync(); -}, []); +useEffect(() => { + if (reload !== "true") { + markAllAsReadMutation.mutateAsync(); + } +}, [reload]);
106-122: Rename for clarity: this is a near-top/prepend trigger, not “bottom intersection”.Reduces cognitive load; e.g.,
triggerFetchOlder()oronNearTop().
124-139: Re-initialize scroll on filter changes.If the list never hits length 0 during a filter swap,
hasInitializedScrollRefstays true and you won’t auto-scroll for the new dataset. Reset on filter change:useEffect(() => { hasInitializedScrollRef.current = false; }, [activeFilter?.cause]);
79-83: Show meaningful error text.Casting to string can yield “[object Object]”. Use
.messagefallback toString(error).- setToast({ - message: error as unknown as string, - type: "error", - }); + setToast({ + message: error instanceof Error ? error.message : String(error), + type: "error", + });
213-227: Avoid nested scroll containers (double overflow).Outer and inner divs both have
overflow-y-autoand scrollbar classes. This can cause double scrollbars and visual artifacts (e.g., scroll-shadow mismatch). Prefer a single scrollable container.- <div - className="tw-relative tw-flex tw-flex-col tw-rounded-t-xl tw-overflow-y-auto tw-overflow-x-hidden tw-scrollbar-thin tw-scrollbar-thumb-iron-500 tw-scrollbar-track-iron-800 desktop-hover:hover:tw-scrollbar-thumb-iron-300 scroll-shadow" + <div + className="tw-relative tw-flex tw-flex-col tw-rounded-t-xl scroll-shadow" style={notificationsViewStyle}>Keep the inner container as the only scrollable element (already has overflow+scrollbar classes).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
__tests__/app/tools/block-finder/page.client.test.tsx(4 hunks)components/brain/feed/FeedScrollContainer.tsx(2 hunks)components/brain/notifications/NotificationItem.tsx(3 hunks)components/brain/notifications/NotificationItems.tsx(2 hunks)components/brain/notifications/Notifications.tsx(5 hunks)components/brain/notifications/NotificationsWrapper.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- components/brain/notifications/NotificationItem.tsx
- components/brain/notifications/NotificationsWrapper.tsx
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{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 propsUse TypeScript for implementation code
Files:
__tests__/app/tools/block-finder/page.client.test.tsxcomponents/brain/feed/FeedScrollContainer.tsxcomponents/brain/notifications/NotificationItems.tsxcomponents/brain/notifications/Notifications.tsx
**/*.tsx
📄 CodeRabbit inference engine (.cursorrules)
**/*.tsx: Use FontAwesome for icons
Use TailwindCSS for stylingUse React functional components with hooks
Files:
__tests__/app/tools/block-finder/page.client.test.tsxcomponents/brain/feed/FeedScrollContainer.tsxcomponents/brain/notifications/NotificationItems.tsxcomponents/brain/notifications/Notifications.tsx
**/{__tests__/**/*.{ts,tsx},*.test.tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/{__tests__/**/*.{ts,tsx},*.test.tsx}: Place tests in tests directories or alongside components as ComponentName.test.tsx
Mock external dependencies and APIs in tests
Files:
__tests__/app/tools/block-finder/page.client.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__/app/tools/block-finder/page.client.test.tsx
🧬 Code graph analysis (1)
components/brain/notifications/Notifications.tsx (3)
components/common/SpinnerLoader.tsx (1)
SpinnerLoader(8-57)components/brain/my-stream/layout/MyStreamNoItems.tsx (1)
MyStreamNoItems(4-126)components/brain/notifications/NotificationsWrapper.tsx (1)
NotificationsWrapper(21-79)
⏰ 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)
components/brain/feed/FeedScrollContainer.tsx (2)
87-89: Good: clearThrottle helper improves clarity.The
clearThrottlehelper function makes the intent clear and reduces code duplication. Well done.
20-20: Confirm scroll threshold values FeedScrollContainer.tsx uses 200px while Notifications.tsx uses 160px—verify this difference is intentional and aligns with the desired scroll behavior.__tests__/app/tools/block-finder/page.client.test.tsx (3)
7-8: LGTM!The constants centralize date/time values used across multiple test utilities, improving maintainability and reducing duplication.
116-122: LGTM!Using the centralized constants makes the test utility more maintainable and consistent with other parts of the test suite.
217-218: LGTM!Replacing hardcoded timestamps with the
getTimestamphelper makes expectations timezone-agnostic, improving test reliability across different execution environments.Also applies to: 268-268
components/brain/notifications/NotificationItems.tsx (1)
49-60: Memoization predicate looks good.Shallow equality on array reference and handlers is appropriate here.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
components/brain/notifications/NotificationItems.tsx (1)
23-35: Consider removing the index fallback for truly stable keys.Using
indexas a fallback (line 26) still causes instability when older items are prepended, because notifications shift positions and their keys change. Ifnotification.idcan beundefined, consider generating a stable identifier (e.g., a UUID attached to the notification object) before rendering, rather than relying on positional index.If
notification.idis always present, remove the fallback:- const keySuffix = notification.id ?? index; + const keySuffix = notification.id;Otherwise, generate stable keys upstream when notifications are fetched.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
components/brain/constants.ts(1 hunks)components/brain/feed/FeedScrollContainer.tsx(4 hunks)components/brain/notifications/NotificationItems.tsx(2 hunks)components/brain/notifications/Notifications.tsx(6 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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 propsUse TypeScript for implementation code
Files:
components/brain/constants.tscomponents/brain/feed/FeedScrollContainer.tsxcomponents/brain/notifications/Notifications.tsxcomponents/brain/notifications/NotificationItems.tsx
**/*.tsx
📄 CodeRabbit inference engine (.cursorrules)
**/*.tsx: Use FontAwesome for icons
Use TailwindCSS for stylingUse React functional components with hooks
Files:
components/brain/feed/FeedScrollContainer.tsxcomponents/brain/notifications/Notifications.tsxcomponents/brain/notifications/NotificationItems.tsx
🧬 Code graph analysis (2)
components/brain/feed/FeedScrollContainer.tsx (1)
components/brain/constants.ts (1)
NEAR_TOP_SCROLL_THRESHOLD_PX(1-1)
components/brain/notifications/Notifications.tsx (5)
components/brain/constants.ts (1)
NEAR_TOP_SCROLL_THRESHOLD_PX(1-1)components/common/SpinnerLoader.tsx (1)
SpinnerLoader(8-57)components/brain/my-stream/layout/MyStreamNoItems.tsx (1)
MyStreamNoItems(4-126)components/brain/notifications/NotificationsWrapper.tsx (1)
NotificationsWrapper(21-79)components/brain/notifications/NotificationsCauseFilter.tsx (1)
NotificationsCauseFilter(31-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 (23)
components/brain/constants.ts (1)
1-1: LGTM!The constant is well-named and the 200px threshold value is reasonable for detecting near-top scrolling behavior.
components/brain/notifications/Notifications.tsx (12)
3-4: LGTM!The new imports are appropriate for the scroll handling implementation.
Also applies to: 21-21
31-35: LGTM!The refs are appropriately used for tracking scroll state without triggering re-renders.
53-69: LGTM!The mutation correctly handles both
Errorinstances and other error types with improved error messaging.
71-78: LGTM!The effect correctly prevents duplicate
markAllAsReadcalls using a ref-based guard.
115-128: LGTM!The function correctly prepares for prepending by storing the scroll height and setting the prepending flag.
130-144: LGTM!The layout effect correctly initializes scroll position to the bottom for a notifications view.
146-152: LGTM!The effect appropriately resets scroll state when the items list is cleared.
154-156: LGTM!The effect correctly resets scroll initialization when the notification filter changes.
158-175: LGTM!The layout effect correctly maintains scroll position when prepending items by adjusting for the height delta.
181-192: LGTM!The scroll handler correctly detects near-top scrolling and triggers pagination with appropriate guards.
194-216: LGTM!The rendering logic cleanly handles the three states with appropriate components and props.
218-235: LGTM!The JSX structure correctly implements a custom scroll container with proper ref and event handler attachments.
components/brain/feed/FeedScrollContainer.tsx (5)
10-10: LGTM!The import and selector constant are correctly defined for feed item tracking.
Also applies to: 21-21
41-42: LGTM!The refs are appropriately used for tracking intersection observer state.
45-79: LGTM!The mutation observer correctly maintains scroll position when content height changes.
81-217: LGTM!The intersection and mutation observer setup correctly tracks feed items entering and leaving the viewport. The cleanup is comprehensive and prevents memory leaks.
219-273: Past issue resolved: scroll position is now fresh.The scroll handler now correctly reads
scrollTopinside the throttled callback (line 230), ensuringisNearTopreflects the current scroll position rather than a stale value. The logic for triggeringonScrollUpNearTopbased on proximity or out-of-view count is sound.components/brain/notifications/NotificationItems.tsx (5)
1-1: LGTM!The imports are appropriate for the memoization implementation.
16-22: LGTM!The internal component naming clearly distinguishes it from the memoized export.
37-52: LGTM!The rendering structure correctly uses the stable keys and DOM ids from the memoized array.
54-65: LGTM!The memoization with custom comparison correctly prevents unnecessary re-renders by checking reference equality of all props.
67-69: LGTM!The display name and export structure follow React best practices for memoized components.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
components/brain/notifications/NotificationItems.tsx (1)
23-35: Add fallback for undefinednotification.idto prevent duplicate keys and DOM IDs.Lines 26 and 31 use
notification.iddirectly without checking if it's defined. If any notification lacks anid, you'll generate duplicate keys (notification-undefined) and DOM IDs (feed-item-undefined), breaking React's reconciliation and causing potential focus/state issues.Apply this diff to add a stable fallback:
const keyedNotifications = useMemo( () => - items.map((notification) => { - const keySuffix = notification.id; + items.map((notification, index) => { + const keySuffix = notification.id ?? `fallback-${index}`; return { notification, key: `notification-${keySuffix}`, domId: `feed-item-${keySuffix}`, }; }), [items] );Note: If notifications can have persistent identifiers beyond
id, prefer those overindex. The index is used here only as a last resort fallback when no stable identifier exists.
🧹 Nitpick comments (2)
components/brain/notifications/Notifications.tsx (2)
117-130: Memoize triggerFetchOlder with useCallback.Creating a new function on every render is unnecessary since it only depends on stable values (refs, fetchNextPage, hasNextPage, isFetchingNextPage).
Apply this diff:
+ const triggerFetchOlder = useCallback(() => { - const triggerFetchOlder = () => { if (isFetchingNextPage) { return; } if (!hasNextPage) { return; } const container = scrollContainerRef.current; if (container) { previousScrollHeightRef.current = container.scrollHeight; } isPrependingRef.current = true; fetchNextPage(); - }; + }, [isFetchingNextPage, hasNextPage, fetchNextPage]);
183-194: Memoize handleScroll with useCallback.Creating a new function reference on every render is unnecessary and could impact performance, especially with frequent re-renders.
Apply this diff:
+ const handleScroll: UIEventHandler<HTMLDivElement> = useCallback((event) => { - const handleScroll: UIEventHandler<HTMLDivElement> = (event) => { if (!shouldEnableInfiniteScroll) { return; } const container = event.currentTarget; if (container.scrollTop <= NEAR_TOP_SCROLL_THRESHOLD_PX) { if (!isFetchingNextPage && hasNextPage) { triggerFetchOlder(); } } - }; + }, [shouldEnableInfiniteScroll, isFetchingNextPage, hasNextPage, triggerFetchOlder]);Note: Apply this after memoizing
triggerFetchOlder.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
components/brain/notifications/NotificationItems.tsx(2 hunks)components/brain/notifications/Notifications.tsx(6 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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 propsUse TypeScript for implementation code
Files:
components/brain/notifications/Notifications.tsxcomponents/brain/notifications/NotificationItems.tsx
**/*.tsx
📄 CodeRabbit inference engine (.cursorrules)
**/*.tsx: Use FontAwesome for icons
Use TailwindCSS for stylingUse React functional components with hooks
Files:
components/brain/notifications/Notifications.tsxcomponents/brain/notifications/NotificationItems.tsx
🧬 Code graph analysis (1)
components/brain/notifications/Notifications.tsx (4)
components/brain/constants.ts (1)
NEAR_TOP_SCROLL_THRESHOLD_PX(1-1)components/common/SpinnerLoader.tsx (1)
SpinnerLoader(8-57)components/brain/my-stream/layout/MyStreamNoItems.tsx (1)
MyStreamNoItems(4-126)components/brain/notifications/NotificationsWrapper.tsx (1)
NotificationsWrapper(21-79)
⏰ 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 (12)
components/brain/notifications/NotificationItems.tsx (2)
54-65: LGTM! Memoization wrapper is correct.The custom comparison function correctly performs reference equality checks on all props, which will prevent unnecessary re-renders when parent re-renders but props haven't changed.
54-65: Memo comparison is appropriate given parent memoizes callbacks.The strict reference-equality comparison for all props (including callbacks) works correctly because
NotificationsWrappermemoizesonReply,onQuote, andonDropContentClickwithuseCallback(per relevant code snippets). This prevents unnecessary re-renders while maintaining correctness.components/brain/notifications/Notifications.tsx (10)
71-80: LGTM! Initial mark-as-read logic is correct.The effect correctly guards against duplicate mark-as-read calls using the ref and skips execution when reload is present.
98-115: LGTM! Reload handling dependencies are now complete.The exhaustive-deps issue from the previous review has been properly addressed.
160-177: Scroll restoration logic looks correct.The layout effect properly adjusts
scrollTopto compensate for prepended content height, maintaining the user's perceived scroll position. The use ofuseLayoutEffectensures this happens synchronously before paint, preventing visual flicker.
196-218: LGTM! Conditional rendering is clear and correct.The branching logic properly handles loading, empty, and populated states.
132-146: Incorrect: prepend scroll handling already implemented
The seconduseLayoutEffect(lines 160–174) checksisPrependingRef, computes the height delta, and adjustsscrollTop, preserving scroll position on older-item prepends. No action needed.Likely an incorrect or invalid review comment.
98-115: Dependencies are now correctly listed.This effect now includes all required dependencies
[reload, refetch, markAllAsRead, searchParams, pathname, router], addressing the past review concern about missing dependencies and stale closures.Based on past review comments.
71-80: Initial mark-all-as-read logic is correct.The
hasMarkedAllAsReadRef.currentguard prevents duplicate mark-all-as-read calls on re-renders. The effect depends on[markAllAsRead, reload], which are stable (mutation function) and external (search param) respectively, so this won't cause unnecessary re-executions.
132-177: Verify the complex scroll-restoration logic is resilient to rapid state changes.You have four effects managing scroll state across lines 132–177:
- Initial scroll to bottom (lines 132–146)
- Reset refs on empty items (lines 148–154)
- Reset scroll init on filter change (lines 156–158)
- Scroll-position restoration during prepending (lines 160–177)
While the logic appears sound for a reverse-chronological feed (scroll to bottom initially, load older items when scrolling up, maintain visual position during prepend), the interplay between these effects is complex. Ensure that rapid filter changes, quick fetch completions, or abrupt item updates don't cause race conditions or visual glitches.
Run the following manual test scenarios to confirm robustness:
- Rapid filter changes: Switch between filters quickly while items are loading.
- Quick scrolls near top: Scroll near the top threshold repeatedly to trigger multiple
fetchNextPagecalls.- Empty → non-empty transitions: Clear the feed (via filter) and then load items again.
- Browser resize during prepending: Trigger an older-item fetch, then resize the browser window before the new items render.
Expected: No scroll jumps, no stuck loading states, and smooth visual transitions in all cases.
183-194: Near-top scroll threshold logic is correct.The
handleScrollfunction correctly checks if the user is withinNEAR_TOP_SCROLL_THRESHOLD_PX(200px) from the top and triggerstriggerFetchOlderwith appropriate guards (isFetchingNextPage,hasNextPage). This implements the expected "load older items when scrolling up" behavior for a reverse-chronological notification feed.
196-218: Conditional rendering branches are correct.The three branches (loader, no items, items) are clearly separated and render the appropriate UI for each state. The
NotificationsWrappercorrectly receives the renamedloadingOlderprop (previouslyloading) and all required callbacks.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
components/brain/notifications/Notifications.tsx (4)
40-45: Complex ref-based state management.The multiple refs effectively track scroll state without triggering re-renders. However, this approach increases cognitive complexity. Consider whether some refs could be consolidated or whether alternative patterns (e.g., reducer) might improve maintainability.
142-157: Consider explicit bottom scroll calculation.The scroll-to-bottom implementation works due to browser clamping, but for clarity consider:
- scrollElement.scrollTop = scrollElement.scrollHeight; + scrollElement.scrollTop = scrollElement.scrollHeight - scrollElement.clientHeight;This makes the intent more explicit, though the current implementation is functionally correct.
196-239: Optimize ResizeObserver effect dependencies.The dependency array
[items, showLoader, showNoItems]causes the observer to be recreated frequently. SinceshowLoaderandshowNoItemsare derived fromitems.lengthand other state, they're redundant dependencies. Consider simplifying to[items.length]or stabilizing the observer creation.- }, [items, showLoader, showNoItems]); + }, [items.length]);Or consider creating the observer once and updating only the scroll behavior based on state:
useLayoutEffect(() => { const scrollElement = scrollContainerRef.current; if (!scrollElement || typeof ResizeObserver === "undefined") { return; } const observationTarget = (scrollElement.firstElementChild as HTMLElement | null) ?? scrollElement; let rafId: number | null = null; const scrollToBottom = () => { if (!hasInitializedScrollRef.current || !isPinnedToBottomRef.current) { return; } if (rafId !== null) { cancelAnimationFrame(rafId); } rafId = requestAnimationFrame(() => { const container = scrollContainerRef.current; if (container) { container.scrollTop = container.scrollHeight; } rafId = null; }); }; const observer = new ResizeObserver(scrollToBottom); observer.observe(observationTarget); return () => { if (rafId !== null) { cancelAnimationFrame(rafId); } observer.disconnect(); }; }, []);
241-271: Simplify redundant condition check.Lines 249-253 have a redundant condition. The
else if (distanceFromBottom > STICK_TO_BOTTOM_SCROLL_THRESHOLD_PX)check is unnecessary since it's the inverse of the condition at line 246.- if (isNearBottom) { - isPinnedToBottomRef.current = true; - } else if (distanceFromBottom > STICK_TO_BOTTOM_SCROLL_THRESHOLD_PX) { - isPinnedToBottomRef.current = false; - } + isPinnedToBottomRef.current = isNearBottom;The rest of the scroll logic correctly handles infinite scroll triggering with appropriate guards.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
components/brain/notifications/NotificationItems.tsx(2 hunks)components/brain/notifications/Notifications.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- components/brain/notifications/NotificationItems.tsx
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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 propsUse TypeScript for implementation code
Files:
components/brain/notifications/Notifications.tsx
**/*.tsx
📄 CodeRabbit inference engine (.cursorrules)
**/*.tsx: Use FontAwesome for icons
Use TailwindCSS for stylingUse React functional components with hooks
Files:
components/brain/notifications/Notifications.tsx
🧬 Code graph analysis (1)
components/brain/notifications/Notifications.tsx (4)
components/brain/constants.ts (1)
NEAR_TOP_SCROLL_THRESHOLD_PX(1-1)components/common/SpinnerLoader.tsx (1)
SpinnerLoader(8-57)components/brain/my-stream/layout/MyStreamNoItems.tsx (1)
MyStreamNoItems(4-126)components/brain/notifications/NotificationsWrapper.tsx (1)
NotificationsWrapper(21-79)
⏰ 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 (10)
components/brain/notifications/Notifications.tsx (10)
3-28: LGTM!The new imports support the custom scroll container implementation. All added hooks and types are utilized appropriately throughout the component.
30-30: LGTM!The 32px threshold for bottom-pinning is reasonable and provides a good user experience for detecting when the user is at the bottom of the scroll container.
63-79: LGTM!The improved error handling now displays
error.messagewhen available, providing more helpful feedback to users.
81-90: Consider React StrictMode behavior.The guard using
hasMarkedAllAsReadRefprevents duplicate calls, but in development with StrictMode, the effect may run twice before the ref is checked. This is acceptable since it's a dev-only behavior, but be aware that the flag is set during the effect without cleanup.
108-125: LGTM! Past review concern addressed.The dependency array now correctly includes all dependencies (
reload,refetch,markAllAsRead,searchParams,pathname,router), fixing the exhaustive-deps violation from the previous review.
127-140: LGTM!The callback is properly guarded and captures scroll height before triggering the fetch. The null check on
scrollContainerRef.currentprevents runtime errors.
159-171: LGTM!The scroll state resets are correctly triggered when items become empty or when the filter changes, ensuring consistent scroll behavior across state transitions.
173-190: LGTM!The scroll position restoration after prepending older items is correctly implemented. The delta calculation maintains the user's visual position, and
useLayoutEffectprevents visual jumps.
273-295: LGTM!The conditional rendering logic cleanly handles all states (loading, empty, items). The prop rename from
loadingtoloadingOlderimproves clarity.
297-314: LGTM!The custom scroll container implementation provides full control over scroll behavior while maintaining clean structure. The scrollbar styling is appropriately configured.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
__tests__/components/brain/feed/FeedScrollContainer.test.tsx (1)
70-98: Use threshold constants in tests instead of hardcoded values
• Import and referenceNEAR_TOP_SCROLL_THRESHOLD_PX(and the bottom‐threshold constant) in your test setup rather than hardcoding50/701to keep tests aligned with implementation.
• Simulate IntersectionObserver callback entries to verify out‐of‐view logic instead of relying on its default stub.
🧹 Nitpick comments (5)
__tests__/components/brain/notifications/Notifications.test.tsx (5)
96-113: Make loader assertion more resilient (avoid element selector and use findByText)Using selector: 'div' with getByText is brittle. Prefer a user-centric text query and async findBy*.
- expect(screen.getByText('Loading notifications...', { selector: 'div' })).toBeInTheDocument(); + expect(await screen.findByText(/Loading notifications/i)).toBeInTheDocument();
116-133: Strengthen “items” branch assertionsAlso assert the filter is rendered and the “no items” fallback is absent to solidify branch coverage.
render(<Notifications activeDrop={null} setActiveDrop={jest.fn()} />); expect(screen.getByTestId('wrapper')).toBeInTheDocument(); + expect(screen.getByTestId('filter')).toBeInTheDocument(); + expect(screen.queryByTestId('no-items')).not.toBeInTheDocument(); await waitFor(() => { expect(mutateAsyncMock).toHaveBeenCalled(); });
135-152: Strengthen “no items” branch assertionsAlso ensure the wrapper is not present when showing the empty state.
render(<Notifications activeDrop={null} setActiveDrop={jest.fn()} />); expect(screen.getByTestId('no-items')).toBeInTheDocument(); + expect(screen.queryByTestId('wrapper')).not.toBeInTheDocument(); await waitFor(() => { expect(mutateAsyncMock).toHaveBeenCalled(); });
46-49: Remove unused BrainContentInput mockThis mock isn’t used in these tests; safe to drop for clarity.
-jest.mock('@/components/brain/content/input/BrainContentInput', () => ({ - __esModule: true, - default: () => <div data-testid="input" />, -}));
88-95: Prefer global mock cleanup with afterEachUse jest.clearAllMocks() in afterEach to avoid manual per-mock clearing and reduce repetition.
describe('Notifications component', () => { beforeEach(() => { - mutateAsyncMock.mockClear(); mutateAsyncMock.mockResolvedValue(undefined); useNotificationsQueryMock.mockReset(); - setTitleMock.mockClear(); }); + + afterEach(() => { + jest.clearAllMocks(); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
__tests__/components/brain/NotificationsWrapper.test.tsx(1 hunks)__tests__/components/brain/feed/FeedScrollContainer.test.tsx(1 hunks)__tests__/components/brain/notifications/Notifications.test.tsx(5 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 propsUse TypeScript for implementation code
Files:
__tests__/components/brain/feed/FeedScrollContainer.test.tsx__tests__/components/brain/NotificationsWrapper.test.tsx__tests__/components/brain/notifications/Notifications.test.tsx
**/*.tsx
📄 CodeRabbit inference engine (.cursorrules)
**/*.tsx: Use FontAwesome for icons
Use TailwindCSS for stylingUse React functional components with hooks
Files:
__tests__/components/brain/feed/FeedScrollContainer.test.tsx__tests__/components/brain/NotificationsWrapper.test.tsx__tests__/components/brain/notifications/Notifications.test.tsx
**/{__tests__/**/*.{ts,tsx},*.test.tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/{__tests__/**/*.{ts,tsx},*.test.tsx}: Place tests in tests directories or alongside components as ComponentName.test.tsx
Mock external dependencies and APIs in tests
Files:
__tests__/components/brain/feed/FeedScrollContainer.test.tsx__tests__/components/brain/NotificationsWrapper.test.tsx__tests__/components/brain/notifications/Notifications.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__/components/brain/feed/FeedScrollContainer.test.tsx__tests__/components/brain/NotificationsWrapper.test.tsx__tests__/components/brain/notifications/Notifications.test.tsx
__tests__/components/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (tests/AGENTS.md)
Use
@testing-library/reactand@testing-library/user-eventfor React component tests
Files:
__tests__/components/brain/feed/FeedScrollContainer.test.tsx__tests__/components/brain/NotificationsWrapper.test.tsx__tests__/components/brain/notifications/Notifications.test.tsx
🧬 Code graph analysis (2)
__tests__/components/brain/NotificationsWrapper.test.tsx (1)
components/brain/notifications/NotificationsWrapper.tsx (1)
NotificationsWrapper(21-79)
__tests__/components/brain/notifications/Notifications.test.tsx (1)
components/brain/notifications/Notifications.tsx (1)
Notifications(37-315)
⏰ 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 (3)
__tests__/components/brain/NotificationsWrapper.test.tsx (2)
29-36: LGTM! Prop rename and loading text correctly updated.The test correctly reflects the API change from
loadingtoloadingOlderand the updated loading message. The regex pattern appropriately matches the component's actual text "Loading older notifications..." without being overly strict about punctuation.
42-47: LGTM! Second test case consistently updated.The test correctly uses the renamed
loadingOlder={false}prop, maintaining consistency with the first test case and the component's updated API.__tests__/components/brain/feed/FeedScrollContainer.test.tsx (1)
30-32: LGTM!The cleanup properly removes the IntersectionObserver mock from the global scope, preventing test pollution.
2 participants
Summary by CodeRabbit
New Features
Refactor
Performance
Tests