Skip to content

Conversation

@emlimlf
Copy link
Collaborator

@emlimlf emlimlf commented Nov 22, 2024

Changes proposed ✍️

What

  • Used the activity count included in the organization property in the organization profile overview page
  • Added activity count column on the People > Overview table
  • Added a new property for the filter.vue component called exclude filter list. This common component is processing the "segmentId" in the URL and parsing it as a filter which causes an error.
  • Modified the filter value assignment to check for the missing required properties (ex: order)
  • Fix the search string passing on the organizations > people tab

Why

These changes fixes the bugs stated on this ticket: https://linear.app/lfx/issue/LFX-1803/issues-in-people-and-organization-profiles

How

copilot:walkthrough

Checklist ✅

  • Label appropriately with Feature, Improvement, or Bug.
  • Add screenshots to the PR description for relevant FE changes
  • New backend functionality has been unit-tested.
  • API documentation has been updated (if necessary) (see docs on API documentation).
  • Quality standards are met.

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced a search functionality for filtering contributors in the organization details.
    • Added a new "Activities" table header in contributor project details.
  • Improvements

    • Enhanced readability and layout of various components, including contributor and organization details.
    • Improved formatting for popovers and activity counts for better clarity.
  • Bug Fixes

    • Streamlined data-fetching processes and error handling in several components.
  • Documentation

    • Updated prop and method signatures for consistency and clarity across components.

@emlimlf emlimlf requested a review from joanagmaia November 22, 2024 09:42
@coderabbitai
Copy link

coderabbitai bot commented Nov 22, 2024

Walkthrough

The changes primarily involve modifications to several Vue components, focusing on enhancing readability and structural clarity in templates and scripts. Key updates include the introduction of new props and variables for managing search functionality, the refinement of table layouts, and the simplification of control flow in data-fetching functions. The adjustments aim to improve the user interface and maintain functionality without altering the underlying logic.

Changes

File Change Summary
frontend/src/modules/contributor/components/details/overview/contributor-details-projects.vue Enhanced template layout with a new "Activities" header, improved text formatting in popovers, and refined table row structure. Minor script formatting adjustments made.
frontend/src/modules/organization/components/details/organization-details-contributors.vue Introduced searchStr prop for filtering contributors, updated fetch function for search integration, and simplified pagination logic.
frontend/src/modules/organization/components/details/overview/organization-details-community.vue Adjusted whitespace for consistency, simplified loading state logic, and removed unnecessary variables/functions for clarity.
frontend/src/shared/modules/filters/components/Filter.vue Improved readability of template and script sections, reorganized imports, and added segmentId variable for query parameters.
frontend/src/shared/modules/filters/components/FilterItem.vue Enhanced formatting and readability with optional chaining, updated props and emit signatures to use semicolons.

Possibly related PRs

Suggested labels

Bug

Suggested reviewers

  • themarolt

🐰 "In the fields where bunnies play,
Changes sprout like flowers in May.
With clearer views and layouts bright,
Our code now dances in the light!
Let's hop along, improve the way,
For every project, come what may!" 🐇

Warning

Rate limit exceeded

@emlimlf has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 26 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 9fb7d90 and 45c5ef2.


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 9fb7d90 and 45c5ef2.

📒 Files selected for processing (1)
  • frontend/src/shared/modules/filters/components/Filter.vue (6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • frontend/src/shared/modules/filters/components/Filter.vue

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🧹 Outside diff range and nitpick comments (11)
frontend/src/modules/organization/components/details/overview/organization-details-community.vue (2)

72-72: Add type safety for activityCount

Consider adding null check for better type safety.

-const activityCount = computed<number>(() => props.organization.activityCount);
+const activityCount = computed<number>(() => props.organization.activityCount ?? 0);

Line range hint 78-82: Consider type safety for segments handling

The segments filter could benefit from type safety improvements.

-      segments: selectedSegment.value ? [selectedSegment.value] : props.organization.segments,
+      segments: selectedSegment.value ? [selectedSegment.value] : (props.organization.segments ?? []),
frontend/src/shared/modules/filters/components/FilterItem.vue (4)

21-26: Consider simplifying the complex template expression

The template expression could be moved to a computed property for better readability and maintainability.

Consider refactoring to:

- v-html="
-   $sanitize(
-     (props.modelValue && config.itemLabelRenderer(props.modelValue, props.config.options, data)) ||
-       `<span class='!text-gray-500'>${config.label}...</span>`
-   )
- "
+ v-html="$sanitize(labelContent)"

+ // In script section
+ const labelContent = computed(() => {
+   return props.modelValue && config.itemLabelRenderer(props.modelValue, props.config.options, data) ||
+     `<span class='!text-gray-500'>${config.label}...</span>`;
+ });

31-39: Add error handling for component loading

The component rendering could be more robust with error boundaries.

Consider adding error handling:

- <component
-   :is="getComponent"
-   v-if="getComponent"
-   v-model="form"
-   v-model:data="data"
-   :config="props.config"
-   v-bind="props.config.options"
- />
+ <component
+   :is="getComponent"
+   v-if="getComponent"
+   v-model="form"
+   v-model:data="data"
+   :config="props.config"
+   v-bind="props.config.options"
+   @error="handleComponentError"
+ >
+   <template #fallback>
+     <div class="p-4 text-sm text-gray-500">
+       Failed to load filter component
+     </div>
+   </template>
+ </component>

Line range hint 87-95: Add consistent null checking in computed setter

While the getter uses optional chaining, the setter assumes props.config.id exists. This could lead to runtime errors.

Consider adding null checks in the setter:

 const isOpen = computed({
   get() {
     return props.open === props.config?.id;
   },
   set(value) {
     if (value) {
-      emit('update:open', props.config.id);
+      emit('update:open', props.config?.id ?? '');
-    } else if (props.config.id === props.open) {
+    } else if (props.config?.id === props.open) {
       emit('update:open', '');
     }
   },
 });

118-124: Add null check for form value assignment

The form value assignment should handle cases where the value is null or undefined.

Consider adding a null check:

 watch(
   () => props.modelValue,
   (value) => {
-    form.value = { ...value };
+    form.value = value ? { ...value } : {};
   },
   { immediate: true, deep: true }
 );
frontend/src/modules/contributor/components/details/overview/contributor-details-projects.vue (1)

156-170: Consider simplifying the affiliations aggregation logic.

The current implementation using reduce could be simplified for better readability while maintaining the same functionality.

Consider this alternative implementation:

-const getAffiliations = (projectId: string) =>
-  (props.contributor.affiliations || [])
-    .filter((affiliation) => affiliation.segmentId === projectId)
-    .reduce((obj: Record<string, ContributorAffiliation[]>, aff: ContributorAffiliation) => {
-      if (!obj[aff.organizationId]) {
-        return {
-          ...obj,
-          [aff.organizationId]: [aff],
-        };
-      }
-      return {
-        ...obj,
-        [aff.organizationId]: [...obj[aff.organizationId], aff],
-      };
-    }, {});
+const getAffiliations = (projectId: string) => {
+  const projectAffiliations = (props.contributor.affiliations || [])
+    .filter((affiliation) => affiliation.segmentId === projectId);
+  
+  return projectAffiliations.reduce((obj: Record<string, ContributorAffiliation[]>, aff) => ({
+    ...obj,
+    [aff.organizationId]: [...(obj[aff.organizationId] || []), aff],
+  }), {});
+};
frontend/src/shared/modules/filters/components/Filter.vue (2)

157-193: Consider splitting watch logic into separate concerns

The watch implementation handles multiple responsibilities (fetching, URL updates, and event tracking). This could make it harder to test and maintain.

Consider extracting these into separate composable functions:

// useFilterUrlSync.ts
export function useFilterUrlSync(filters: Ref<Filter>, hash?: string) {
  const router = useRouter();
  
  watch(filters, (value) => {
    const query = setQuery(value);
    router.push({ query, hash: hash ? `#${hash}` : undefined });
  }, { deep: true });
}

// useFilterEventTracking.ts
export function useFilterEventTracking(filters: Ref<Filter>) {
  const router = useRouter();
  const { trackEvent } = useProductTracking();
  
  watch(filters, (value) => {
    const key = determineEventKey(router.currentRoute.value);
    if (key) {
      trackEvent({
        key,
        type: EventType.FEATURE,
        properties: { path: router.currentRoute.value.path, filter: value },
      });
    }
  }, { deep: true });
}

Then use them in the component:

useFilterUrlSync(filters, props.hash);
useFilterEventTracking(filters);
watch(filters, fetch, { deep: true });

167-177: Make event tracking more maintainable

The event key determination is tightly coupled with specific routes and uses hard-coded values. This makes it fragile to route changes.

Consider using a route-to-event mapping configuration:

const ROUTE_EVENT_MAP = {
  member: FeatureEventKey.FILTER_MEMBERS,
  organization: FeatureEventKey.FILTER_ORGANIZATIONS,
  activity: {
    '#activity': FeatureEventKey.FILTER_ACTIVITIES,
    '#conversation': FeatureEventKey.FILTER_CONVERSATIONS
  }
} as const;

const determineEventKey = (route: RouteLocationNormalizedLoaded): FeatureEventKey | null => {
  const routeConfig = ROUTE_EVENT_MAP[route.name as keyof typeof ROUTE_EVENT_MAP];
  if (!routeConfig) return null;
  
  return typeof routeConfig === 'object' 
    ? routeConfig[route.hash as keyof typeof routeConfig] ?? null
    : routeConfig;
};
frontend/src/modules/organization/components/details/organization-details-contributors.vue (2)

181-181: Type safety improvement needed

The searchStr ref and savedBody ref could benefit from more specific typing rather than relying on type inference and any.

Consider adding explicit types:

-const savedBody = ref<any>({});
-const searchStr = ref<string>('');
+interface FilterBody {
+  // Add appropriate filter body properties
+  organizations?: { contains: string[] };
+  // ... other filter properties
+}
+const savedBody = ref<FilterBody>({});
+const searchStr = ref<string>('');

Also applies to: 269-270


Line range hint 1-297: Consider implementing error boundary and loading states management

While the component implements the required features, consider these architectural improvements:

  1. Implement an error boundary component to handle failed API calls gracefully
  2. Consider using a state management pattern for loading states to prevent race conditions
  3. Add retry logic for failed API calls
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 6ea0694 and ae2b0b8.

📒 Files selected for processing (6)
  • frontend/src/modules/contributor/components/details/overview/contributor-details-projects.vue (6 hunks)
  • frontend/src/modules/organization/components/details/organization-details-contributors.vue (10 hunks)
  • frontend/src/modules/organization/components/details/overview/organization-details-community.vue (4 hunks)
  • frontend/src/shared/modules/filters/components/Filter.vue (7 hunks)
  • frontend/src/shared/modules/filters/components/FilterItem.vue (4 hunks)
  • scripts/scaffold.yaml (1 hunks)
🔇 Additional comments (16)
frontend/src/modules/organization/components/details/overview/organization-details-community.vue (3)

24-31: LGTM: Improved activity count loading and display logic

The changes properly handle the loading state using activitySycning.state and provide appropriate fallback display. The number formatting is correctly applied.


37-41: LGTM: Robust date handling

Good improvement in date handling with proper edge case management for default dates.


72-72: Verify activity count integration

Let's verify the activity count integration across related components.

✅ Verification successful

Let me search more broadly since the Vue file type wasn't recognized, and also check for any GraphQL operations that might be fetching this data.


Activity count integration is properly implemented

The activityCount property is well integrated across the codebase:

  • Correctly typed in Organization.ts interface as a number
  • Used consistently in organization components for display and sorting
  • Properly computed in the component under review
  • Integrated with filters, saved views, and list components
  • Has corresponding test coverage for filtering scenarios
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for other components using activityCount
rg -t vue "activityCount" frontend/src/modules/organization/

# Check for any TypeScript interfaces defining activityCount
rg -t ts "activityCount" frontend/src/modules/organization/types/

Length of output: 235


Script:

#!/bin/bash
# Search for activityCount usage across all files in frontend
rg "activityCount" frontend/

# Check for any GraphQL queries or mutations related to activityCount
rg -g "*.graphql" "activityCount" frontend/

Length of output: 11380

frontend/src/shared/modules/filters/components/FilterItem.vue (2)

70-81: LGTM! Clean TypeScript declarations

The props and emit declarations are well-structured and follow TypeScript best practices.


134-138: LGTM! Clean styling implementation

The styles are well-structured and follow Tailwind best practices.

scripts/scaffold.yaml (2)

143-143: Good practice: Using a specific Kafka image version.

Pinning to a specific version (3.8.0-debian-12-r6) instead of using latest improves build reproducibility and makes version management more predictable.


Line range hint 143-164: Verify the removal of KAFKA_KRAFT_CLUSTER_ID environment variable.

The removal of KAFKA_KRAFT_CLUSTER_ID environment variable needs verification since Kafka is running in KRaft mode (as indicated by PROCESS_ROLES=controller,broker). This ID is typically required for cluster identification.

Let's verify the Kafka configuration and any potential issues:

Please ensure that:

  1. The Kafka service starts correctly without this environment variable
  2. The KRaft consensus protocol works as expected
  3. There are no cluster identification issues in logs during startup
✅ Verification successful

Let me try another verification approach focusing on the Docker Compose configuration and Kafka environment variables:


Let me check the Kafka configurations in both files to understand the complete setup:


The removal of KAFKA_KRAFT_CLUSTER_ID is safe with this Bitnami Kafka image

The Bitnami Kafka image (3.8.0-debian-12-r6) automatically generates a random cluster ID if not explicitly set. Both the development (scaffold.yaml) and test (docker-compose.test.yaml) configurations are consistent in not specifying this variable, while properly configuring other essential KRaft mode settings:

  • Controller role is properly set (KAFKA_CFG_PROCESS_ROLES=controller,broker)
  • Controller listener and quorum voters are correctly configured
  • All necessary security protocols and listeners are defined
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if KAFKA_KRAFT_CLUSTER_ID is required and if there are any related issues reported

# Search for any references to KRAFT_CLUSTER_ID in documentation or code
rg -i "kraft.*cluster.*id"

# Check if there are any related issues or discussions
gh issue list --search "kafka kraft cluster"
gh pr list --search "kafka kraft cluster"

# Check the Kafka image documentation for version 3.8.0
curl -s https://hub.docker.com/v2/repositories/bitnami/kafka/tags/3.8.0-debian-12-r6 | jq '.description'

Length of output: 229


Script:

#!/bin/bash
# Search for any Kafka configuration files or docker-compose files
fd -e yaml -e yml | xargs grep -l "kafka"

# Check if there are any other Kafka configurations in the codebase
rg -l "KAFKA_CFG" 

# Look for any documentation about Kafka setup
fd "README" | xargs grep -l -i "kafka"

Length of output: 221


Script:

#!/bin/bash
# Check both files containing Kafka configurations
echo "=== scaffold.yaml ==="
cat scripts/scaffold.yaml

echo -e "\n=== docker-compose.test.yaml ==="
cat backend/docker-compose.test.yaml

Length of output: 6490

frontend/src/modules/contributor/components/details/overview/contributor-details-projects.vue (5)

18-22: LGTM! Clear and professional descriptions.

The updated descriptions for Affiliation and Role (Maintainer/Contributor) are well-written and provide clear explanations of these concepts.

Also applies to: 34-42


47-47: LGTM! Activity count integration.

The new Activities column and its implementation align with the PR objectives, displaying the activity count from the organization property.

Also applies to: 77-79


172-177: LGTM! Clean computed property and helper function.

The refactored projects computed property and getMaintainerRoles helper function are concise and maintain good readability.

Also applies to: 179-180


144-144: Verify type safety of project mapping.

Let's ensure proper typing for the project spread operation and affiliations assignment.

Also applies to: 174-175

✅ Verification successful

Type safety is properly implemented in the component

The component's type safety is well-maintained:

  • The Contributor interface is properly defined and imported from the types module
  • Project mapping is type-safe through computed properties and proper interface usage
  • Affiliations and maintainer roles are correctly typed and filtered
  • The component properly handles the spread of project properties with additional type-safe affiliations mapping
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for Segment/Project type definitions
ast-grep --pattern 'type $_Segment = {
  $$$
}'

# Search for related interfaces
ast-grep --pattern 'interface $_Project {
  $$$
}'

Length of output: 102


Script:

#!/bin/bash
# Let's try a broader search for type definitions and interfaces
rg -A 5 "type\s+(?:Segment|Project|Contributor)" --type ts

# Search for any Project-related type definitions
rg -A 5 "interface.*Project" --type ts

# Look for the actual component usage
rg -B 2 -A 5 "contributor:\s*Contributor" --type vue

Length of output: 3931


Script:

#!/bin/bash
# Let's look for the Contributor type definition and its usage in the component
rg -A 5 "type\s+Contributor|interface\s+Contributor" --type ts

# Look at the component implementation to understand the project mapping
cat "frontend/src/modules/contributor/components/details/overview/contributor-details-projects.vue"

Length of output: 12511


94-100: Verify type safety of the report data modal.

The report issue modal implementation looks good, but let's verify the type safety of the data being passed.

✅ Verification successful

Let me gather more information about the type safety by checking the implementation and usage of the setReportDataModal function.


Let me check the report data modal component and its type definitions to complete the verification.


Type safety of report data modal is properly implemented

The implementation shows proper type safety with:

  • ReportDataType enum properly defines PROJECT_AFFILIATION
  • setReportDataModal action is typed with an interface accepting contributor, type, and attribute props
  • The modal component has proper TypeScript props validation and handles the project affiliation data correctly
  • The form submission properly processes and validates the data before sending
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for ReportDataType and related interfaces/types
ast-grep --pattern 'enum ReportDataType {
  $$$
  PROJECT_AFFILIATION = $_,
  $$$
}'

# Search for setReportDataModal usage and typing
ast-grep --pattern 'setReportDataModal($_)'

Length of output: 1819


Script:

#!/bin/bash
# Search for setReportDataModal implementation and type definition
rg "setReportDataModal" -A 5

# Search for the report data modal component and its props/interface
fd "report.*modal" --type f

# Search for the report data interface
ast-grep --pattern 'interface ReportData {
  $$$
}'

Length of output: 13217


Script:

#!/bin/bash
# Check the report data modal component implementation
cat frontend/src/shared/modules/report-issue/component/report-data-issue-modal.vue

# Search for any interfaces or types related to the report data
rg "type ReportData" -A 5
rg "interface ReportData" -A 5

Length of output: 8262

frontend/src/shared/modules/filters/components/Filter.vue (2)

39-39: LGTM: Clean styling implementation

The class binding provides a consistent appearance for the operator div while maintaining good readability.


86-86: Address temporary implementation and clarify filter usage

The excludeFilters prop is marked as a temporary fix, and there's a TODO comment about verifying the parsed filter usage. This could lead to technical debt.

Consider:

  1. Documenting the specific use cases for excludeFilters
  2. Creating a ticket to track the proper implementation
  3. Adding tests to verify the behavior

Also applies to: 216-216

frontend/src/modules/organization/components/details/organization-details-contributors.vue (2)

9-9: LGTM: Filter exclusion implementation

The addition of :exclude-filters prop aligns with the PR objective to fix the segmentId URL processing issue.


126-132: Validate pagination boundary condition

The pagination condition should also check if there are any contributors to prevent showing the "Load more" section when there are no results.

Consider updating the condition to include a check for contributors:

-v-if="pagination.total > pagination.page * pagination.perPage"
+v-if="contributors.length > 0 && pagination.total > pagination.page * pagination.perPage"

Comment on lines 253 to 269
const removeExcludedFilters = (filter: Filter) => {
const tmpFilter = { ...filter };
if (props.excludeFilters) {
props.excludeFilters.forEach((filterKey: string) => {
delete tmpFilter[filterKey];
});
}
return tmpFilter;
};
const fixSavedFilterValue = (saved: Filter, model: Filter) => {
if (!saved.order) {
return { ...model, ...saved };
}
return saved;
};
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance filter management functions for better robustness

The new utility functions could benefit from some improvements:

  1. removeExcludedFilters mutates the input object directly
  2. fixSavedFilterValue has implicit behavior based on the order property
  3. Both functions lack error handling for edge cases (null/undefined inputs)

Consider this improved implementation:

-const removeExcludedFilters = (filter: Filter) => {
-  const tmpFilter = { ...filter };
-  if (props.excludeFilters) {
-    props.excludeFilters.forEach((filterKey: string) => {
-      delete tmpFilter[filterKey];
-    });
-  }
-  return tmpFilter;
-};
+const removeExcludedFilters = (filter: Filter): Filter => {
+  if (!filter) return {};
+  return Object.fromEntries(
+    Object.entries(filter).filter(([key]) => 
+      !props.excludeFilters?.includes(key)
+    )
+  );
+};

-const fixSavedFilterValue = (saved: Filter, model: Filter) => {
-  if (!saved.order) {
-    return { ...model, ...saved };
-  }
-  return saved;
-};
+const fixSavedFilterValue = (saved: Filter, model: Filter): Filter => {
+  if (!saved || !model) return {};
+  return saved.order ? saved : { ...model, ...saved };
+};
📝 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 removeExcludedFilters = (filter: Filter) => {
const tmpFilter = { ...filter };
if (props.excludeFilters) {
props.excludeFilters.forEach((filterKey: string) => {
delete tmpFilter[filterKey];
});
}
return tmpFilter;
};
const fixSavedFilterValue = (saved: Filter, model: Filter) => {
if (!saved.order) {
return { ...model, ...saved };
}
return saved;
};
const removeExcludedFilters = (filter: Filter): Filter => {
if (!filter) return {};
return Object.fromEntries(
Object.entries(filter).filter(([key]) =>
!props.excludeFilters?.includes(key)
)
);
};
const fixSavedFilterValue = (saved: Filter, model: Filter): Filter => {
if (!saved || !model) return {};
return saved.order ? saved : { ...model, ...saved };
};

Comment on lines 185 to 186
// TODO: need to revisit this. Understand why the filter.vue is parsing the segmentId for this component as a filter in that component
const excludeFilters = ['segmentId'];
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Address TODO comment with proper documentation

The TODO comment indicates uncertainty about the segmentId parsing. This should be properly documented or resolved rather than leaving it as a TODO.

Please either:

  1. Document why segmentId needs to be excluded and remove the TODO
  2. Implement a proper fix if this is a workaround

Comment on lines 234 to +237
filter: {
and: [
orgFilter,
savedBody.value,
],
and: [orgFilter, savedBody.value],
},
search: searchStr.value,
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 error handling in fetch function

The fetch function resets data on error but doesn't provide user feedback about the failure.

Consider adding error handling:

 filter: {
   and: [orgFilter, savedBody.value],
 },
 search: searchStr.value,
+}).catch((error) => {
+  contributors.value = [];
+  pagination.value.total = 0;
+  // Add error notification
+  console.error('Failed to fetch contributors:', error);
+  // If you have a notification system
+  // notify.error('Failed to load contributors. Please try again.');

Committable suggestion skipped: line range outside the PR's diff.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (2)
frontend/src/modules/organization/components/details/organization-details-contributors.vue (2)

229-231: Add error handling for member count fetch

The count fetching logic should include error handling to maintain UI consistency in case of API failures.

  MemberService.listMembers(
    {
      limit: 1,
      offset: 0,
      filter: orgFilter,
      segments: selectedProjectGroup.value?.id ? [selectedProjectGroup.value?.id] : props.organization.segments,
    },
    true,
-  ).then(({ count }) => {
-    totalContacts.value = count;
-  });
+  ).then(({ count }) => {
+    totalContacts.value = count;
+  }).catch((error) => {
+    console.error('Failed to fetch member count:', error);
+    totalContacts.value = 0;
+  });

Line range hint 1-300: Consider component decomposition and state management optimization

The component handles multiple responsibilities (filtering, pagination, GDPR compliance). Consider:

  1. Extracting the pagination logic into a reusable composable
  2. Moving the filter state management to a dedicated store
  3. Creating a separate component for the contributor list item

This would improve maintainability and reusability while following the Single Responsibility Principle.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 82aabfd and 9fb7d90.

📒 Files selected for processing (4)
  • frontend/src/modules/organization/components/details/organization-details-contributors.vue (9 hunks)
  • frontend/src/shared/modules/filters/components/Filter.vue (6 hunks)
  • frontend/src/shared/modules/filters/components/FilterItem.vue (4 hunks)
  • scripts/scaffold.yaml (0 hunks)
💤 Files with no reviewable changes (1)
  • scripts/scaffold.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
  • frontend/src/shared/modules/filters/components/Filter.vue
  • frontend/src/shared/modules/filters/components/FilterItem.vue
🔇 Additional comments (3)
frontend/src/modules/organization/components/details/organization-details-contributors.vue (3)

23-23: UI improvements enhance user experience and accessibility

The template changes improve visual consistency and user feedback through:

  • Better sort label visibility
  • Consistent icon sizing
  • Clear GDPR data masking indication
  • Improved pagination UI with loading states

Also applies to: 38-38, 67-67, 109-109, 127-133


159-159: Clean implementation of search functionality

The addition of searchStr ref and component import follow Vue 3 composition API best practices.

Also applies to: 184-184


264-264: Clean implementation of filter and pagination logic

The changes simplify the pagination condition while maintaining correct functionality, and properly handle filter updates including the search string.

Also applies to: 272-273

Signed-off-by: Efren Lim <[email protected]>
Signed-off-by: Efren Lim <[email protected]>
Copy link
Contributor

@joanagmaia joanagmaia left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@emlimlf emlimlf merged commit 7380b43 into main Nov 28, 2024
6 checks passed
@emlimlf emlimlf deleted the bugfix/lfx-1803-people-tab-org branch November 28, 2024 00:22
@coderabbitai coderabbitai bot mentioned this pull request Jan 7, 2025
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants