fix(web): reset infinite scroll when notification filters change #966
Conversation
WalkthroughThe changes in the Changes
Poem
🪧 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 (
|
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
![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
🧹 Outside diff range and nitpick comments (4)
web/components/Notifications/List.vue (4)
27-29: Consider making the watcher more specific to filter changesThe current watcher resets
canLoadMoreon any prop change, includingpageSize. Consider watching only the filter-related props to avoid unnecessary resets.-watch(props, () => { +watch([() => props.type, () => props.importance], () => { canLoadMore.value = true; });
Line range hint
38-40: Enhance error handling for better user experienceWhile error logging is implemented, the UI doesn't reflect error states. Consider showing an error message to users when loading fails.
+const showError = ref(false); + watch(error, (newVal) => { console.log('[getNotifications] error:', newVal); + if (newVal) { + showError.value = true; + canLoadMore.value = false; + } });And in the template:
<div v-if="notifications?.length > 0" v-infinite-scroll="[onLoadMore, { canLoadMore: () => canLoadMore }]" class="divide-y divide-gray-200 overflow-y-auto pl-7 pr-4 h-full" > + <div v-if="showError" class="text-red-600 p-2 text-center"> + Failed to load more notifications. Please try again later. + </div> <NotificationsItem
Line range hint
52-64: Add loading state to prevent duplicate requestsThe current implementation might trigger multiple simultaneous requests if the user scrolls quickly while a request is in progress.
+const isLoading = ref(false); + async function onLoadMore() { + if (isLoading.value) return; console.log('[getNotifications] onLoadMore'); + isLoading.value = true; + try { const incoming = await fetchMore({ variables: { filter: { offset: notifications.value.length, limit: props.pageSize, type: props.type, importance: props.importance, }, }, }); const incomingCount = incoming?.data.notifications.list.length ?? 0; if (incomingCount === 0 || incomingCount < props.pageSize) { canLoadMore.value = false; } + } finally { + isLoading.value = false; + } }
Line range hint
76-77: Add loading indicator for better user feedbackConsider adding a loading indicator to show when more notifications are being fetched.
<div v-if="notifications?.length > 0" v-infinite-scroll="[onLoadMore, { canLoadMore: () => canLoadMore }]" class="divide-y divide-gray-200 overflow-y-auto pl-7 pr-4 h-full" > <NotificationsItem v-for="notification in notifications" :key="notification.id" v-bind="notification" /> + <div v-if="isLoading" class="py-2 text-center text-gray-500"> + Loading more notifications... + </div> </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
web/components/Notifications/List.vue(1 hunks)
🔇 Additional comments (1)
web/components/Notifications/List.vue (1)
24-26: LGTM: Well-documented state management
The new canLoadMore ref is appropriately named and documented, clearly indicating its purpose in controlling the infinite scroll behavior.
fixes a bug where infinite scroll wouldn't trigger after it stopped, even when changing filters.
Summary by CodeRabbit
New Features
Bug Fixes