Fix activity pagination, load more functionality #2671
Conversation
WalkthroughThe changes in this pull request include the introduction of a new component, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ActivitiesComponent
participant ScrollControll
User->>ActivitiesComponent: Scroll down
ActivitiesComponent->>ScrollControll: Check scroll position
ScrollControll-->>ActivitiesComponent: Emit bottom event
ActivitiesComponent->>ActivitiesComponent: Call loadMore()
ActivitiesComponent->>ActivitiesComponent: Fetch more activities
ActivitiesComponent-->>User: Display additional activities
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (13)
frontend/src/ui-kit/scrollcontroll/ScrollControll.vue (1)
1-5: Consider enhancing accessibility and customization options.The scrollable container should have proper ARIA attributes for better accessibility. Also, consider making the scroll threshold configurable.
<template> - <div class="relative overflow-auto" v-bind="$attrs" @scroll="controlScroll"> + <div + class="relative overflow-auto" + role="region" + aria-label="Scrollable content" + v-bind="$attrs" + @scroll="controlScroll" + > <slot /> </div> </template>frontend/src/modules/data-quality/pages/data-quality.page.vue (1)
Line range hint
2-39: Consider implementing virtual scrolling for better performance.Given that this PR focuses on pagination and load more functionality, and this component serves as a parent for data-heavy views (People and Organizations), it might benefit from virtual scrolling implementation.
Consider:
- Using a virtual scrolling library like
vue-virtual-scrollerfor the child components- Implementing pagination state management at this level to share common pagination logic between tabs
- Adding a loading state indicator for better UX during data fetching
Would you like assistance in implementing any of these suggestions?
frontend/src/modules/organization/components/details/organization-details-activities.vue (1)
46-54: Consider extracting pagination logic into a composableSince this pagination pattern is being implemented across multiple components (organization-details-activities, contributor-details-activities), consider creating a reusable composable.
Example approach:
// useActivityPagination.ts import { ref } from 'vue' import type { AppActivityTimeline } from '@/modules/activity/components/activity-timeline.vue' export function useActivityPagination() { const timeline = ref<AppActivityTimeline | null>(null) const loading = ref(false) const loadMore = async () => { if (!timeline.value || loading.value) return try { loading.value = true await timeline.value.fetchActivities() } catch (error) { console.error('Failed to load more activities:', error) } finally { loading.value = false } } return { timeline, loading, loadMore } }This would:
- Reduce code duplication
- Ensure consistent behavior across components
- Make it easier to maintain and modify pagination logic
frontend/src/modules/contributor/components/details/contributor-details-activities.vue (2)
57-57: Add type safety to timeline refConsider adding type safety to the timeline ref for better TypeScript integration.
-const timeline = ref(null); +const timeline = ref<InstanceType<typeof AppActivityTimeline> | null>(null);
67-69: Consider exposing loading stateThe loading state should be exposed to parent components for better UX coordination.
defineExpose({ loadMore, + isLoading, });frontend/src/modules/data-quality/components/member/data-quality-member-issues.vue (2)
148-152: Consider improving the scroll container styles.A few suggestions to enhance maintainability and prevent potential issues:
- Add scoping to prevent style conflicts
- Document the magic number (228px) calculation
- Consider using CSS variables for better maintainability
-<style lang="scss"> +<style lang="scss" scoped> +// Height calculation: +// 228px = Header height (64px) + Padding (40px) + Tabs (48px) + Additional spacing (76px) +$header-offset: 228px; + .scroll-container{ - max-height: calc(100vh - 228px); + max-height: calc(100vh - #{$header-offset}); } </style>
Line range hint
6-39: Consider adding error handling for load more functionality.While the implementation works well, it might benefit from error handling to improve user experience when loading fails.
Consider adding error state handling:
<lf-scroll-controll v-else-if="members.length > 0" class="scroll-container -mb-4" @bottom="loadMore()"> + <div v-if="error" class="text-red-500 text-sm text-center py-2"> + Failed to load more items. Please try again. + </div> <!-- existing content --> </lf-scroll-controll>And in the script:
const error = ref(false); const loadDataIssues = () => { error.value = false; loading.value = true; // ... existing code ... .catch(() => { error.value = true; }) .finally(() => { loading.value = false; }); };frontend/src/modules/data-quality/components/organization/data-quality-organization-merge-suggestions.vue (2)
Line range hint
6-28: Consider adding loading state for infinite scroll.While there's a loading indicator for the initial load, there's no visual feedback when loading more items through scrolling. This might confuse users about whether more content is being loaded.
Consider adding a loading indicator at the bottom of the scroll container:
<lf-scroll-controll v-else-if="mergeSuggestions.length > 0" class="scroll-container -mb-4" @bottom="loadMore()"> <!-- ... existing content ... --> + <div v-if="loadingMore" class="flex justify-center py-4"> + <lf-spinner size="small" /> + </div> </lf-scroll-controll>
136-140: Improve scroll container height calculation.The current implementation uses a magic number (228px) which makes maintenance difficult and might not work well across different screen sizes and layouts.
Consider these improvements:
- Use CSS custom properties for better maintainability:
- max-height: calc(100vh - 228px); + max-height: calc(100vh - var(--header-height) - var(--padding-top) - var(--padding-bottom));
- Add responsive behavior:
.scroll-container { - max-height: calc(100vh - 228px); + max-height: calc(100vh - 228px); + @media (max-width: 768px) { + max-height: calc(100vh - 180px); + } }
- Scope the styles to prevent conflicts:
-<style lang="scss"> +<style lang="scss" scoped>frontend/src/modules/data-quality/components/member/data-quality-member-merge-suggestions.vue (1)
153-157: Document the magic number in the height calculation.The
calc(100vh - 228px)uses a magic number that should be documented. Consider:
- Adding a comment explaining how 228px was calculated
- Using CSS variables for better maintainability
Here's a suggested improvement:
<style lang="scss"> +/* 228px breakdown: + * - Header height: XXpx + * - Padding: XXpx + * - Other elements: XXpx + */ +:root { + --header-offset: 228px; +} .scroll-container{ - max-height: calc(100vh - 228px); + max-height: calc(100vh - var(--header-offset)); } </style>frontend/src/modules/contributor/pages/contributor-details.page.vue (2)
136-136: Consider using proper type instead of 'any'Replace
anywith the proper component type for better type safety and IDE support.-const activities = ref<any>(null); +const activities = ref<InstanceType<typeof LfContributorDetailsActivities> | null>(null);
82-82: Consider decoupling scroll handling from parent componentThe current implementation couples scroll handling with the parent component. Consider:
- Moving scroll handling to individual tab components
- Using a reusable scroll container component
- Implementing an event-based approach for load more functionality
This would improve component reusability and maintain better separation of concerns.
Also applies to: 136-136, 158-159
frontend/src/modules/organization/pages/organization-details.page.vue (1)
139-140: Consider adding TypeScript types for refs.While the ref initialization is correct, consider adding explicit types for better type safety:
-const contributors = ref(null); -const activities = ref(null); +const contributors = ref<InstanceType<typeof LfOrganizationDetailsContributors> | null>(null); +const activities = ref<InstanceType<typeof LfOrganizationDetailsActivities> | null>(null);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
frontend/src/modules/activity/components/activity-timeline.vue(8 hunks)frontend/src/modules/contributor/components/details/contributor-details-activities.vue(3 hunks)frontend/src/modules/contributor/pages/contributor-details.page.vue(3 hunks)frontend/src/modules/data-quality/components/member/data-quality-member-issues.vue(4 hunks)frontend/src/modules/data-quality/components/member/data-quality-member-merge-suggestions.vue(4 hunks)frontend/src/modules/data-quality/components/organization/data-quality-organization-merge-suggestions.vue(4 hunks)frontend/src/modules/data-quality/pages/data-quality.page.vue(1 hunks)frontend/src/modules/organization/components/details/organization-details-activities.vue(2 hunks)frontend/src/modules/organization/pages/organization-details.page.vue(3 hunks)frontend/src/ui-kit/scrollcontroll/ScrollControll.vue(1 hunks)
🔇 Additional comments (19)
frontend/src/ui-kit/scrollcontroll/ScrollControll.vue (1)
1-21: Verify consistent usage across the application.
Let's ensure this component is being used consistently across all implementations.
✅ Verification successful
Let me gather more information about the actual usage patterns in these files.
Let me try a different approach using case-insensitive search and template syntax:
Component usage is consistent across all implementations
The verification shows consistent usage patterns across all three implementations:
- All components use kebab-case
lf-scroll-controllin templates - All implementations correctly use the
@bottomevent handler - All components consistently bind it to a
loadMore()function - All have the same conditional rendering pattern with
v-else-ifand classscroll-container
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage patterns of LfScrollControll component
# Expected: Consistent usage of @bottom event and proper attribute passing
# Search for component usage
echo "Checking component usage patterns..."
rg -A 3 "LfScrollControll" frontend/src/modules/
# Check for potential scroll handling in other components that might need migration
echo "Checking for other scroll handlers that might need migration..."
ast-grep --pattern 'onScroll($$$) { $$$ }' frontend/src/modules/
Length of output: 1856
Script:
#!/bin/bash
# Check the actual implementation and usage in the identified files
echo "Checking data-quality-member-merge-suggestions.vue..."
rg -A 10 "<LfScrollControll" frontend/src/modules/data-quality/components/member/data-quality-member-merge-suggestions.vue
echo -e "\nChecking data-quality-organization-merge-suggestions.vue..."
rg -A 10 "<LfScrollControll" frontend/src/modules/data-quality/components/organization/data-quality-organization-merge-suggestions.vue
echo -e "\nChecking data-quality-member-issues.vue..."
rg -A 10 "<LfScrollControll" frontend/src/modules/data-quality/components/member/data-quality-member-issues.vue
Length of output: 728
Script:
#!/bin/bash
# Search for component usage with different possible template syntaxes
echo "Checking component usage with various syntaxes..."
rg -i -A 10 "<(lf-scroll-controll|lfscrollcontroll)" frontend/src/modules/data-quality/components/
# Also check for kebab-case usage in template
echo -e "\nChecking for kebab-case event handling..."
rg -i "@bottom" frontend/src/modules/data-quality/components/
# Check template section specifically
echo -e "\nChecking template sections..."
rg -p "<template>[\s\S]*?</template>" frontend/src/modules/data-quality/components/member/data-quality-member-merge-suggestions.vue
rg -p "<template>[\s\S]*?</template>" frontend/src/modules/data-quality/components/organization/data-quality-organization-merge-suggestions.vue
rg -p "<template>[\s\S]*?</template>" frontend/src/modules/data-quality/components/member/data-quality-member-issues.vue
Length of output: 6043
frontend/src/modules/data-quality/pages/data-quality.page.vue (1)
2-2: Verify the impact of padding removal on scroll functionality.
The removal of bottom padding using !pb-0 might be related to fixing scroll detection issues, but using the !important modifier (via Tailwind's ! prefix) could make the styling harder to maintain or override when needed.
Consider these alternatives:
- If this is for scroll detection, consider handling it in the scroll controller component instead of modifying padding
- If padding adjustment is necessary, consider using a more specific selector or a CSS custom property for better maintainability
Let's verify if other components have similar padding adjustments:
✅ Verification successful
Let me gather more context about the usage of these components and their scroll-related functionality.
The padding removal is intentional and correctly implemented
The !pb-0 class on app-page-wrapper is a necessary change that works in conjunction with the LfScrollControll component used in child components. The scroll containers in child components (data-quality-member-issues, data-quality-member-merge-suggestions, and data-quality-organization-merge-suggestions) all use a negative margin bottom (-mb-4) and have a calculated max height. Removing the parent's padding ensures proper scroll detection and consistent spacing.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar padding modifications in other components
rg -g '*.vue' '!pb-0'
Length of output: 251
Script:
#!/bin/bash
# Check for scroll-related code in these components and their parent directories
rg -g '*.vue' -B 2 -A 2 'scroll' frontend/src/modules/automation frontend/src/modules/data-quality
# Look for LfScrollControl usage as mentioned in PR context
rg -g '*.vue' 'LfScrollControl' frontend/src/modules/automation frontend/src/modules/data-quality
Length of output: 8457
frontend/src/modules/organization/components/details/organization-details-activities.vue (1)
25-25: LGTM: Timeline ref properly integrated
The ref addition to app-activity-timeline is correctly implemented, enabling proper access to the component instance for pagination control.
frontend/src/modules/contributor/components/details/contributor-details-activities.vue (2)
30-30: LGTM: Timeline ref properly configured
The ref addition to AppActivityTimeline is correctly implemented, enabling proper interaction with the timeline component instance.
67-69: Verify parent component integration
The loadMore method is correctly exposed, but let's verify its integration with the parent component's scroll handling.
✅ Verification successful
Integration with parent component is properly implemented
The exposed loadMore method is correctly integrated in the parent component contributor-details.page.vue. The parent implements a scroll handler that calls activities.value.loadMore() when the 'activities' tab is active and the scroll reaches the bottom. This matches the expected integration pattern mentioned in the review.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of loadMore in parent components
rg -l "loadMore.*contributor-details-activities" frontend/src/modules/contributor/
Length of output: 84
Script:
#!/bin/bash
# Let's try a broader search for the component name and potential parent components
rg -l "contributor-details-activities" frontend/src/modules/contributor/
# Also search for loadMore usage in potential parent components
rg "loadMore" frontend/src/modules/contributor/ -A 3 -B 3
Length of output: 4372
frontend/src/modules/data-quality/components/member/data-quality-member-issues.vue (2)
67-67: LGTM! Import statement is correctly placed.
The LfScrollControll component is properly imported and integrates well with the existing load more functionality.
Line range hint 6-39: LGTM! Verify the negative margin's impact.
The scroll control implementation looks good and properly integrates with the existing member issues list. The infinite scroll trigger is correctly implemented using the @bottom event.
Please verify if the negative margin (-mb-4) affects the visual layout:
✅ Verification successful
Negative margin usage is consistent with similar components
The -mb-4 class is consistently used with lf-scroll-controll components across similar data quality views, including organization and member merge suggestions. This establishes a clear pattern for scroll container spacing in the application.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances of negative margins in similar components
rg -l "class=\".*-mb-4.*\"" "frontend/src/modules/**/*.vue"
Length of output: 129
Script:
#!/bin/bash
# Let's try searching without the glob pattern which might not be supported
# Search for negative margins in the frontend directory
rg -l "class=\".*-mb-4.*\"" frontend/
Length of output: 382
Script:
#!/bin/bash
# Let's examine the context of -mb-4 usage in these files to understand the pattern
rg "class=\".*-mb-4.*\"" frontend/src/modules/data-quality/components/ -C 2
Length of output: 2230
frontend/src/modules/data-quality/components/organization/data-quality-organization-merge-suggestions.vue (1)
6-6: Verify ScrollControll component implementation.
Let's ensure the ScrollControll component is implemented correctly and used consistently across the application.
Also applies to: 65-65
✅ Verification successful
ScrollControll component is correctly implemented and consistently used
The verification confirms:
- The ScrollControll component is properly implemented in
frontend/src/ui-kit/scrollcontroll/ScrollControll.vuewith correct scroll event handling and bottom emission - It's consistently used across data quality components for both organization and member sections
- The component name 'LfScrollControll' matches the usage in the reviewed code
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check ScrollControll component implementation
echo "Checking ScrollControll component implementation..."
cat frontend/src/ui-kit/scrollcontroll/ScrollControll.vue || echo "ScrollControll component not found at expected location"
# Check for consistent usage across the application
echo -e "\nChecking usage patterns..."
rg -l "LfScrollControll" frontend/src/
Length of output: 1052
frontend/src/modules/data-quality/components/member/data-quality-member-merge-suggestions.vue (2)
71-71: LGTM!
The import is correctly placed and follows the project's component organization pattern.
Line range hint 6-31: LGTM! Consider edge cases for scroll behavior.
The implementation of LfScrollControll looks good, providing both scroll-based and button-based loading. However, let's verify the scroll behavior in edge cases.
Please verify these scenarios:
- Rapid scrolling doesn't trigger multiple simultaneous requests
- Loading state is properly handled during scroll-triggered loads
- The component behaves correctly with different viewport sizes
✅ Verification successful
Scroll behavior implementation is properly handled
After reviewing the LfScrollControll component implementation, I can confirm that:
- The scroll logic is simple and robust, using a 10px threshold to trigger the bottom event
- The component emits the 'bottom' event only when actually reaching the bottom, preventing multiple simultaneous requests
- The implementation is viewport-size independent as it uses relative measurements (scrollTop + clientHeight vs scrollHeight)
The scroll behavior is well-implemented and doesn't require any modifications. The original review comment's concerns about edge cases are addressed by the clean implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for LfScrollControll implementation to understand how it handles these edge cases
ast-grep --pattern 'export default {
$$$
methods: {
$$$
onScroll($_) {
$$$
}
$$$
}
$$$
}'
Length of output: 121
Script:
#!/bin/bash
# Let's try to find the LfScrollControll component implementation
fd -e vue -e js "scroll-controll" -X cat
# Also search for any file containing scroll-related logic
rg -i "onscroll|scrollcontroll" --type vue --type js -C 3
Length of output: 128
Script:
#!/bin/bash
# Let's try different variations of the component name and fix the file type for rg
fd "scroll|Scroll" -e vue -e js -X cat
# Search with correct file extension
rg -i "onscroll|scrollcontrol" -t js -t vue -C 3
Length of output: 1547
frontend/src/modules/contributor/pages/contributor-details.page.vue (1)
82-82: LGTM: Ref properly added to activities component
The ref addition enables proper interaction with the activities component for load more functionality.
frontend/src/modules/organization/pages/organization-details.page.vue (2)
94-94: LGTM: Template ref addition is correct.
The addition of ref="activities" to the activities component enables proper integration with the scroll-based load more functionality.
166-167: Verify loadMore implementation consistency.
Let's verify that the loadMore method is properly implemented in the activities component and follows the same pattern as other components.
✅ Verification successful
Based on the search results, I can see that the loadMore() method is consistently implemented across various components in the codebase, including:
- Organization details page uses it for loading more activities and contributors
- Member merge suggestions page uses it for pagination
- Data quality components use it for member and organization merge suggestions
- Contributor details page uses it for notes and activities
The implementation follows a common pattern where loadMore() is used with infinite scroll or "Load more" buttons to fetch additional data by incrementing the page number and maintaining the existing items.
The implementation is consistent with the codebase patterns. The usage in organization-details.page.vue for loading more activities follows the same pattern as other components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check loadMore implementation in related components
echo "Checking loadMore implementation in activities component..."
ast-grep --pattern 'loadMore() {
$$$
}'
echo "\nChecking for similar pagination implementations..."
rg -A 10 'loadMore\(\)' --type vue
Length of output: 352
Script:
#!/bin/bash
# Let's try a different approach to find loadMore implementations
echo "Searching for loadMore method implementations..."
rg -A 10 'loadMore\(\)'
echo "\nSearching for files containing 'loadMore'..."
rg -l 'loadMore'
echo "\nChecking for pagination-related patterns..."
rg -A 5 'pagination|pageSize|currentPage'
Length of output: 167499
frontend/src/modules/activity/components/activity-timeline.vue (6)
87-87: LGTM: Simplified rendering condition
The simplified condition for rendering timeline items is more maintainable and correctly handles the empty state elsewhere in the template.
220-226: LGTM: Migrated to design system button component
The change from el-button to lf-button maintains functionality while improving UI consistency with the design system.
Also applies to: 264-264
346-348: LGTM: Added safety guard against concurrent fetches
The loading check prevents race conditions and duplicate requests, improving reliability.
453-456: LGTM: Clean component interface exposure
Exposing fetchActivities enables the load more functionality required by the PR objectives while maintaining a minimal public interface.
379-384: Consider handling activities older than one month
The one-month window for fetching activities might limit access to historical data. Consider:
- Making the time window configurable
- Adding a warning when older activities exist but aren't shown
#!/bin/bash
# Check if there are any existing issues or discussions about activity history limitations
rg -i "activity.*history|older.*activities" .github/Also applies to: 417-421
308-313: Verify impact of reduced page size on user experience
The changes to initialization values look good, but the reduction in page size from 50 to 10 items might affect user experience:
- ✅ Faster initial page load
- ❓ More frequent pagination requests
✅ Verification successful
Let me analyze the pagination implementation to better understand the impact of reducing the page size.
Reducing page size from 50 to 10 is safe and beneficial
The implementation in activity-list.vue shows a robust pagination system with:
- Load more button that appears when more items are available
- Proper handling of append operations for smooth infinite scroll
- Default fetch limit of 50 in the component, making the timeline's reduced limit of 10 a performance optimization
The change is beneficial because:
- Faster initial page load with smaller data set
- Load more functionality ensures users can still access all activities
- No risk of degraded UX as pagination is properly implemented
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any performance-related comments or issues
rg -i "performance|slow|pagination" .github/
Length of output: 54
Script:
#!/bin/bash
# Check activity timeline component for pagination and loading logic
rg -A 5 -B 5 "loadMore|pagination|scroll" frontend/src/modules/activity/components/activity-timeline.vue
# Check for any existing activity list size or performance related tests
rg -i "activity.*items.*length|activity.*limit" frontend/src/modules/activity/
Length of output: 187
Script:
#!/bin/bash
# Search for the loadActivities method implementation
ast-grep --pattern 'async loadActivities() {
$$$
}'
# Look for infinite scroll or load more implementation
ast-grep --pattern 'onScroll($$$) {
$$$
}'
# Check for any API endpoints or services related to activities
rg -l "activities.*limit|per_page" frontend/src/
Length of output: 255
Script:
#!/bin/bash
# Check the content of activity-list.vue for pagination implementation
cat frontend/src/modules/activity/components/activity-list.vue
# Check the content of github config for any pagination defaults
cat frontend/src/integrations/github/config.js
# Look for any scroll or intersection observer implementation in the timeline
rg -A 5 "IntersectionObserver|scroll" frontend/src/modules/activity/components/activity-timeline.vue
Length of output: 7083
| <script setup lang="ts"> | ||
| const emit = defineEmits<{(e: 'bottom'): void}>(); | ||
|
|
||
| const controlScroll = (e: any) => { | ||
| if (e.target.scrollTop + e.target.clientHeight >= e.target.scrollHeight - 10) { | ||
| emit('bottom'); | ||
| } | ||
| }; | ||
| </script> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety and scroll handling efficiency.
The current implementation has several areas for improvement:
- The event type should be properly typed instead of 'any'
- Scroll events should be debounced to prevent performance issues
- The scroll threshold should be configurable
<script setup lang="ts">
-const emit = defineEmits<{(e: 'bottom'): void}>();
+interface Props {
+ threshold?: number;
+ debounceMs?: number;
+}
-const controlScroll = (e: any) => {
- if (e.target.scrollTop + e.target.clientHeight >= e.target.scrollHeight - 10) {
- emit('bottom');
- }
+const props = withDefaults(defineProps<Props>(), {
+ threshold: 10,
+ debounceMs: 200
+});
+
+const emit = defineEmits<{(e: 'bottom'): void}>();
+
+const controlScroll = (e: UIEvent) => {
+ const target = e.target as HTMLElement;
+ if (
+ Math.ceil(target.scrollTop + target.clientHeight) >=
+ target.scrollHeight - props.threshold
+ ) {
+ debounce(() => emit('bottom'), props.debounceMs)();
+ }
};
</script>You'll need to import the debounce utility:
import { debounce } from 'lodash-es';| <script lang="ts"> | ||
| export default { | ||
| name: 'LfScrollControll', | ||
| }; | ||
| </script> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix component naming and modernize component definition.
- The component name has a typo ('Controll' should be 'Control')
- In Vue 3 with
<script setup>, the separate script block for defining the component name is unnecessary
-<script lang="ts">
-export default {
- name: 'LfScrollControll',
-};
-</script>
+<script setup lang="ts">
+defineOptions({
+ name: 'LfScrollControl'
+});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <script lang="ts"> | |
| export default { | |
| name: 'LfScrollControll', | |
| }; | |
| </script> | |
| <script setup lang="ts"> | |
| defineOptions({ | |
| name: 'LfScrollControl' | |
| }); | |
| </script> |
| const timeline = ref<AppActivityTimeline | null>(null); | ||
|
|
||
| const loadMore = () => { | ||
| timeline.value.fetchActivities(); | ||
| }; | ||
|
|
||
| defineExpose({ | ||
| loadMore, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and null checks to loadMore method
While the implementation is functional, it could benefit from additional safeguards to prevent runtime errors and improve user experience.
Consider applying these improvements:
const timeline = ref<AppActivityTimeline | null>(null);
-const loadMore = () => {
- timeline.value.fetchActivities();
-};
+const loading = ref(false);
+
+const loadMore = async () => {
+ if (!timeline.value || loading.value) return;
+
+ try {
+ loading.value = true;
+ await timeline.value.fetchActivities();
+ } catch (error) {
+ console.error('Failed to load more activities:', error);
+ // Consider adding user feedback for the error
+ } finally {
+ loading.value = false;
+ }
+};
defineExpose({
loadMore,
});This implementation:
- Prevents calls when the timeline isn't ready or already loading
- Adds proper error handling
- Manages loading state to prevent duplicate calls
- Makes the method async to properly handle the Promise
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const timeline = ref<AppActivityTimeline | null>(null); | |
| const loadMore = () => { | |
| timeline.value.fetchActivities(); | |
| }; | |
| defineExpose({ | |
| loadMore, | |
| }); | |
| const timeline = ref<AppActivityTimeline | null>(null); | |
| const loading = ref(false); | |
| const loadMore = async () => { | |
| if (!timeline.value || loading.value) return; | |
| try { | |
| loading.value = true; | |
| await timeline.value.fetchActivities(); | |
| } catch (error) { | |
| console.error('Failed to load more activities:', error); | |
| // Consider adding user feedback for the error | |
| } finally { | |
| loading.value = false; | |
| } | |
| }; | |
| defineExpose({ | |
| loadMore, | |
| }); |
| const loadMore = () => { | ||
| timeline.value.fetchActivities(); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and loading state
The loadMore method should include error handling and loading state management.
+const isLoading = ref(false);
+
const loadMore = async () => {
+ if (isLoading.value) return;
+
+ try {
+ isLoading.value = true;
timeline.value.fetchActivities();
+ } catch (error) {
+ console.error('Failed to load more activities:', error);
+ // Consider adding user notification here
+ } finally {
+ isLoading.value = false;
+ }
};Committable suggestion skipped: line range outside the PR's diff.
| @@ -62,6 +62,7 @@ import AppOrganizationMergeSuggestionsDialog | |||
| from '@/modules/organization/components/organization-merge-suggestions-dialog.vue'; | |||
| import { storeToRefs } from 'pinia'; | |||
| import { useLfSegmentsStore } from '@/modules/lf/segments/store'; | |||
| import LfScrollControll from '@/ui-kit/scrollcontroll/ScrollControll.vue'; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prevent race conditions in loadMore function.
The current implementation doesn't prevent multiple simultaneous calls to loadMore(), which could occur when rapidly scrolling or clicking the load more button while a request is in progress.
Consider adding a guard clause:
const loadMore = () => {
+ if (loading.value) return;
offset.value = mergeSuggestions.value.length;
loadMergeSuggestions();
};Also applies to: 108-112
| } else if (tabs.value === 'activities') { | ||
| activities.value.loadMore(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add safeguards to the scroll handling
The current implementation could benefit from additional safeguards:
- Add null check before calling loadMore
- Add debounce/throttle to prevent excessive calls
- Add loading state check to prevent simultaneous calls
+import { debounce } from 'lodash-es';
-const controlScroll = (e) => {
+const controlScroll = debounce((e) => {
if (e.target.scrollTop + e.target.clientHeight >= e.target.scrollHeight - 10) {
if (tabs.value === 'notes') {
- notes.value.loadMore();
+ notes.value?.loadMore?.();
} else if (tabs.value === 'activities') {
- activities.value.loadMore();
+ activities.value?.loadMore?.();
}
}
-};
+}, 200);Committable suggestion skipped: line range outside the PR's diff.
| } else if (tabs.value === 'activities') { | ||
| activities.value.loadMore(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for loadMore calls.
The scroll control logic should handle potential errors when calling loadMore:
} else if (tabs.value === 'activities') {
- activities.value.loadMore();
+ try {
+ activities.value?.loadMore();
+ } catch (error) {
+ console.error('Failed to load more activities:', error);
+ // Consider showing a user-friendly error message
+ }This ensures:
- Null safety with the optional chaining
- Graceful error handling
- Better debugging capability
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } else if (tabs.value === 'activities') { | |
| activities.value.loadMore(); | |
| } else if (tabs.value === 'activities') { | |
| try { | |
| activities.value?.loadMore(); | |
| } catch (error) { | |
| console.error('Failed to load more activities:', error); | |
| // Consider showing a user-friendly error message | |
| } |
Changes proposed ✍️
What
copilot:summary
copilot:poem
Why
How
copilot:walkthrough
Checklist ✅
Feature,Improvement, orBug.Summary by CodeRabbit
Release Notes
New Features
LfScrollControllcomponent for improved scroll handling in various sections.Bug Fixes
Documentation
Style