Fix activity pagination, load more functionality by gaspergrom ยท Pull Request #2671 ยท linuxfoundation/crowd.dev

๐Ÿ”‡ 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-controll in templates
  • All implementations correctly use the @bottom event handler
  • All components consistently bind it to a loadMore() function
  • All have the same conditional rendering pattern with v-else-if and class scroll-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:

  1. If this is for scroll detection, consider handling it in the scroll controller component instead of modifying padding
  2. 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.vue with 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:

  1. Rapid scrolling doesn't trigger multiple simultaneous requests
  2. Loading state is properly handled during scroll-triggered loads
  3. 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:

  1. The scroll logic is simple and robust, using a 10px threshold to trigger the bottom event
  2. The component emits the 'bottom' event only when actually reaching the bottom, preventing multiple simultaneous requests
  3. 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:

  1. Organization details page uses it for loading more activities and contributors
  2. Member merge suggestions page uses it for pagination
  3. Data quality components use it for member and organization merge suggestions
  4. 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:

  1. Making the time window configurable
  2. 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