Skip to content

Fix activity pagination, load more functionality #2671

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 4, 2024
Merged

Fix activity pagination, load more functionality #2671

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 25 additions & 14 deletions frontend/src/modules/activity/components/activity-timeline.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@
</div>
<div>
<el-timeline>
<template v-if="activities.length && !loading">
<template v-if="activities.length">
<el-timeline-item
v-for="activity in activities"
:key="activity.id"
Expand Down Expand Up @@ -217,14 +217,13 @@
class="app-page-spinner"
/>
<div v-if="!noMore" class="flex justify-center pt-4">
<el-button
class="btn btn-link btn-link--primary"
<lf-button
type="primary-ghost"
:disabled="loading"
@click="fetchActivities()"
>
<i class="ri-arrow-down-line mr-2" />Load
more
</el-button>
Load more
</lf-button>
</div>
</div>
</div>
Expand Down Expand Up @@ -262,6 +261,7 @@ import { getSegmentsFromProjectGroup } from '@/utils/segments';
import { Platform } from '@/shared/modules/platform/types/Platform';
import LfActivityDisplay from '@/shared/modules/activity/components/activity-display.vue';
import moment from 'moment';
import LfButton from '@/ui-kit/button/Button.vue';
import { ActivityService } from '../activity-service';

const SearchIcon = h(
Expand Down Expand Up @@ -305,12 +305,12 @@ const enabledPlatforms = computed(() => CrowdIntegrations.enabledConfigs.concat(
label: i.name,
})));

const loading = ref(true);
const loading = ref(false);
const platform = ref(null);
const query = ref('');
const activities = ref([]);
const limit = ref(50);
const timestamp = ref(moment().toISOString());
const limit = ref(10);
const timestamp = ref(moment(props.entity.lastActive).toISOString());
const noMore = ref(false);
const selectedSegment = ref(props.selectedSegment || null);

Expand Down Expand Up @@ -343,6 +343,9 @@ const segments = computed(() => {
});

const fetchActivities = async ({ reset } = { reset: false }) => {
if (loading.value) {
return;
}
const filterToApply = {
platform: platform.value ? { in: [platform.value] } : undefined,
};
Expand Down Expand Up @@ -373,12 +376,12 @@ const fetchActivities = async ({ reset } = { reset: false }) => {
filterToApply.and = [
{
timestamp: {
lte: props.entity?.lastActive ?? timestamp.value,
lte: timestamp.value,
},
},
...(props.entity?.lastActive ? [{
...(timestamp.value ? [{
timestamp: {
gte: moment(props.entity?.lastActive).subtract(1, 'month').toISOString(),
gte: moment(timestamp.value).subtract(1, 'month').toISOString(),
},
}] : []),
];
Expand Down Expand Up @@ -406,12 +409,16 @@ const fetchActivities = async ({ reset } = { reset: false }) => {

const activityIds = activities.value.map((a) => a.id);
const rows = data.rows.filter((a) => !activityIds.includes(a.id));
if (rows.length === 0) {
if (rows.length >= props.entity.activityCount) {
noMore.value = true;
}
activities.value = reset ? rows : [...activities.value, ...rows];

timestamp.value = moment(data.rows.at(-1).timestamp).toISOString();
if (data.rows.length === 0) {
timestamp.value = moment(timestamp.value).subtract(1, 'month').toISOString();
} else {
timestamp.value = moment(data.rows.at(-1).timestamp).toISOString();
}
};

const reloadActivities = async () => {
Expand Down Expand Up @@ -443,6 +450,10 @@ onMounted(async () => {
await store.dispatch('integration/doFetch', segments.value.map((s) => s.id));
await fetchActivities();
});

defineExpose({
fetchActivities,
});
</script>

<script>
Expand Down
13 changes: 12 additions & 1 deletion frontend/src/modules/contributor/components/details/contributor-details-activities.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
</div>
<app-activity-timeline
v-else
ref="timeline"
:entity="props.contributor"
entity-type="member"
:show-affiliations="true"
Expand All @@ -42,7 +43,7 @@ import { useRoute } from 'vue-router';
import { MergeActionState } from '@/shared/modules/merge/types/MemberActions';
import LfIconOld from '@/ui-kit/icon/IconOld.vue';
import useContributorHelpers from '@/modules/contributor/helpers/contributor.helpers';
import { computed } from 'vue';
import { computed, ref } from 'vue';
import LfIcon from '@/ui-kit/icon/Icon.vue';

const props = defineProps<{
Expand All @@ -53,9 +54,19 @@ const route = useRoute();

const { isMasked } = useContributorHelpers();

const timeline = ref(null);

const { subProjectId } = route.query;

const masked = computed(() => isMasked(props.contributor));

const loadMore = () => {
timeline.value.fetchActivities();
};
Comment on lines +63 to +65
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.


defineExpose({
loadMore,
});
</script>

<script lang="ts">
Expand Down
4 changes: 4 additions & 0 deletions frontend/src/modules/contributor/pages/contributor-details.page.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@
/>
<lf-contributor-details-activities
v-else-if="tabs === 'activities'"
ref="activities"
:contributor="contributor"
/>
<lf-contributor-details-notes
Expand Down Expand Up @@ -132,6 +133,7 @@ const route = useRoute();
const tabs = ref('overview');

const notes = ref<any>(null);
const activities = ref<any>(null);

const { id } = route.params;

Expand All @@ -153,6 +155,8 @@ const controlScroll = (e) => {
if (e.target.scrollTop + e.target.clientHeight >= e.target.scrollHeight - 10) {
if (tabs.value === 'notes') {
notes.value.loadMore();
} else if (tabs.value === 'activities') {
activities.value.loadMore();
Comment on lines +158 to +159
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add safeguards to the scroll handling

The current implementation could benefit from additional safeguards:

  1. Add null check before calling loadMore
  2. Add debounce/throttle to prevent excessive calls
  3. 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.

}
}
};
Expand Down
11 changes: 9 additions & 2 deletions frontend/src/modules/data-quality/components/member/data-quality-member-issues.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
<div v-if="loading && offset === 0" class="flex justify-center py-20">
<lf-spinner />
</div>
<div v-else-if="members.length > 0">
<lf-scroll-controll v-else-if="members.length > 0" class="scroll-container -mb-4" @bottom="loadMore()">
<lf-data-quality-member-issues-item
v-for="(member) of members"
:key="member.id"
Expand Down Expand Up @@ -36,7 +36,7 @@
Load more
</lf-button>
</div>
</div>
</lf-scroll-controll>
<div v-else class="flex flex-col items-center pt-16">
<div
class="ri-shuffle-line text-gray-200 text-10xl h-40 flex items-center mb-8"
Expand Down Expand Up @@ -64,6 +64,7 @@ import { storeToRefs } from 'pinia';
import { useLfSegmentsStore } from '@/modules/lf/segments/store';
import { EventType, FeatureEventKey } from '@/shared/modules/monitoring/types/event';
import useProductTracking from '@/shared/modules/monitoring/useProductTracking';
import LfScrollControll from '@/ui-kit/scrollcontroll/ScrollControll.vue';

const props = defineProps<{
type: DataIssueType,
Expand Down Expand Up @@ -143,3 +144,9 @@ export default {
name: 'LfDataQualityMemberIssues',
};
</script>

<style lang="scss">
.scroll-container{
max-height: calc(100vh - 228px);
}
</style>
11 changes: 9 additions & 2 deletions ...tend/src/modules/data-quality/components/member/data-quality-member-merge-suggestions.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
<div v-if="loading && offset === 0" class="flex justify-center py-20">
<lf-spinner />
</div>
<div v-else-if="mergeSuggestions.length > 0">
<lf-scroll-controll v-else-if="mergeSuggestions.length > 0" class="scroll-container -mb-4" @bottom="loadMore()">
<lf-data-quality-member-merge-suggestions-item
v-for="(suggestion, si) of mergeSuggestions"
:key="suggestion.id"
Expand All @@ -28,7 +28,7 @@
Load more
</lf-button>
</div>
</div>
</lf-scroll-controll>
<div v-else class="flex flex-col items-center pt-16">
<div
class="ri-shuffle-line text-gray-200 text-10xl h-40 flex items-center mb-8"
Expand Down Expand Up @@ -68,6 +68,7 @@ import { storeToRefs } from 'pinia';
import { useLfSegmentsStore } from '@/modules/lf/segments/store';
import LfMemberMergeSuggestionDropdown
from '@/modules/member/components/suggestions/member-merge-suggestion-dropdown.vue';
import LfScrollControll from '@/ui-kit/scrollcontroll/ScrollControll.vue';

const props = defineProps<{
projectGroup: string,
Expand Down Expand Up @@ -148,3 +149,9 @@ export default {
name: 'LfDataQualityMemberMergeSuggestions',
};
</script>

<style lang="scss">
.scroll-container{
max-height: calc(100vh - 228px);
}
</style>
11 changes: 9 additions & 2 deletions ...ules/data-quality/components/organization/data-quality-organization-merge-suggestions.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
<div v-if="loading" class="flex justify-center py-20">
<lf-spinner />
</div>
<div v-else-if="mergeSuggestions.length > 0">
<lf-scroll-controll v-else-if="mergeSuggestions.length > 0" class="scroll-container -mb-4" @bottom="loadMore()">
<lf-data-quality-organization-merge-suggestions-item
v-for="(suggestion, si) of mergeSuggestions"
:key="suggestion.id"
Expand All @@ -25,7 +25,7 @@
Load more
</lf-button>
</div>
</div>
</lf-scroll-controll>
<div v-else class="flex flex-col items-center pt-16">
<div
class="ri-shuffle-line text-gray-200 text-10xl h-40 flex items-center mb-8"
Expand Down Expand Up @@ -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';
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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


const props = defineProps<{
projectGroup: string,
Expand Down Expand Up @@ -131,3 +132,9 @@ export default {
name: 'LfDataQualityOrganizationMergeSuggestions',
};
</script>

<style lang="scss">
.scroll-container{
max-height: calc(100vh - 228px);
}
</style>
2 changes: 1 addition & 1 deletion frontend/src/modules/data-quality/pages/data-quality.page.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<template>
<app-page-wrapper>
<app-page-wrapper class="!pb-0">
<div>
<!-- Header -->
<div class="mb-6">
Expand Down
12 changes: 12 additions & 0 deletions frontend/src/modules/organization/components/details/organization-details-activities.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
</div>
<app-activity-timeline
v-else
ref="timeline"
:entity="{
...props.organization,
organizations: [props.organization],
Expand All @@ -36,10 +37,21 @@ import { Organization } from '@/modules/organization/types/Organization';
import AppActivityTimeline from '@/modules/activity/components/activity-timeline.vue';
import LfIconOld from '@/ui-kit/icon/IconOld.vue';
import { MergeActionState } from '@/shared/modules/merge/types/MemberActions';
import { ref } from 'vue';

const props = defineProps<{
organization: Organization,
}>();

const timeline = ref<AppActivityTimeline | null>(null);

const loadMore = () => {
timeline.value.fetchActivities();
};

defineExpose({
loadMore,
});
Comment on lines +46 to +54
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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,
});

</script>

<script lang="ts">
Expand Down
6 changes: 5 additions & 1 deletion frontend/src/modules/organization/pages/organization-details.page.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@
/>
<lf-organization-details-activities
v-else-if="tabs === 'activities'"
ref="activities"
:organization="organization"
/>
</div>
Expand Down Expand Up @@ -135,7 +136,8 @@ const { selectedProjectGroup } = storeToRefs(lsSegmentsStore);
const route = useRoute();

const tabs = ref('overview');
const contributors = ref('overview');
const contributors = ref(null);
const activities = ref(null);
const scrollContainer = ref(null);

const { id } = route.params;
Expand All @@ -161,6 +163,8 @@ const controlScroll = (e: any) => {
if (e.target.scrollTop + e.target.clientHeight >= e.target.scrollHeight - 10) {
if (tabs.value === 'people') {
contributors.value.loadMore();
} else if (tabs.value === 'activities') {
activities.value.loadMore();
Comment on lines +166 to +167
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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:

  1. Null safety with the optional chaining
  2. Graceful error handling
  3. 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.

Suggested change
} 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
}

}
}
};
Expand Down
21 changes: 21 additions & 0 deletions frontend/src/ui-kit/scrollcontroll/ScrollControll.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
<template>
<div class="relative overflow-auto" v-bind="$attrs" @scroll="controlScroll">
<slot />
</div>
</template>

<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>
Comment on lines +7 to +15
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve type safety and scroll handling efficiency.

The current implementation has several areas for improvement:

  1. The event type should be properly typed instead of 'any'
  2. Scroll events should be debounced to prevent performance issues
  3. 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>
Comment on lines +17 to +21
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Fix component naming and modernize component definition.

  1. The component name has a typo ('Controll' should be 'Control')
  2. 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.

Suggested change
<script lang="ts">
export default {
name: 'LfScrollControll',
};
</script>
<script setup lang="ts">
defineOptions({
name: 'LfScrollControl'
});
</script>

Loading