Fix activity pagination, load more functionality by gaspergrom ยท Pull Request #2671 ยท linuxfoundation/crowd.dev
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