Conversation
Signed-off-by: Efren Lim <elim@linuxfoundation.org>
Signed-off-by: Efren Lim <elim@linuxfoundation.org>
Signed-off-by: Efren Lim <elim@linuxfoundation.org>
Signed-off-by: Efren Lim <elim@linuxfoundation.org>
Signed-off-by: Efren Lim <elim@linuxfoundation.org>
WalkthroughThis pull request introduces extensive updates to the application's contributor and organization dependency features. The changes refactor Vue components to incorporate custom metric dropdowns, modular tables, and enhanced loading/error handling. New computed properties and watchers improve dynamic data processing, while additional API endpoints and mock data facilitate backend integration. Updates to shared type definitions and UI kit styles, including a new progress bar component, further improve the overall modularity and maintainability of the codebase. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Contributor Dependency Component
participant API as Contributor Dependency API
participant Toast as Toast Notification
UI->>API: Request dependency data (useFetch)
API-->>UI: Return data or error
alt Error Response
UI->>Toast: Display error notification
UI->>UI: Render error placeholder
else Successful Response
UI->>UI: Compute topContributors & others
UI->>UI: Render metric dropdown and contributors table
end
sequenceDiagram
participant OrgUI as Organization Dependency Component
participant OrgAPI as Organization Dependency API
participant Toast as Toast Notification
OrgUI->>OrgAPI: Request organization dependency data (useFetch)
OrgAPI-->>OrgUI: Return data or error
alt Error Response
OrgUI->>Toast: Display error notification
OrgUI->>OrgUI: Render error placeholder
else Successful Response
OrgUI->>OrgUI: Compute topOrganizations, avatars & others
OrgUI->>OrgUI: Render metric dropdown, organizations table, and dependency display
end
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (17)
frontend/app/components/modules/project/components/contributors/contributor-dependency.vue (6)
9-19: Define an error state or empty state.You have placeholders for the 'error' scenario (lines 16-17). Consider implementing or importing an actual error/empty state component so that the user receives more direct feedback in the event of API failure or no data.
20-44: Ensure fallback for missing contributors.When top or other contributors are empty, consider rendering a fallback message or skeleton UI. This helps avoid confusion when there is insufficient data.
83-86: Add defensive checks for computed properties.When accessing fields like
.topContributorsor.list, ensure the response always includes these properties. Otherwise, default to an empty array to avoid exceptions if the response shape changes.
87-90: Limit avatar generation.Truncating avatars to three for the display group is a good UI decision. If you need to show a "remaining count", you might add a small badge or text (e.g., "+2 more") to show how many were omitted.
91-94: Use consistent labeling for time periods.Lowercasing the time period label is consistent. Just ensure that all references across the UI matches, especially if some design require uppercase.
95-104: Fallback or more proactive error handling.The toast works great for error feedback. You might also index into the error object for more details, or automatically retry once if the error is transient.
frontend/app/components/uikit/progress-bar/progress-bar.stories.ts (1)
1-25: LGTM! Consider adding more stories for different states.The story configuration is well-structured with proper documentation tags and controls. The default story provides a good starting point.
Consider adding more stories to showcase different states:
export const Positive = { args: { value: 75, color: 'positive' } }; export const Warning = { args: { value: 50, color: 'warning' } }; export const Negative = { args: { value: 25, color: 'negative' } };frontend/app/components/modules/project/components/contributors/fragments/metric-dropdown.vue (1)
12-34: Optimize imports and add validation.The component can be improved by optimizing imports and adding validation for the metric options.
-import { computed } from 'vue'; +import { computed, onMounted } from 'vue'; import LfxDropdown from '~/components/uikit/dropdown/dropdown.vue'; import { metricsOptions } from '~/components/shared/types/metrics'; const metricOptions = metricsOptions; +// Validate that modelValue exists in metricOptions +onMounted(() => { + if (!metricOptions.find(option => option.value === props.modelValue)) { + console.warn(`Invalid metric value: ${props.modelValue}`); + } +});frontend/server/api/contributors/organization-dependency.get.ts (1)
1-1: Add type safety for mock data imports.Import types to ensure type safety for the mock data.
+import type { OrganizationDependency } from '~/components/shared/types/contributors.types'; -import { allMetrics, commits } from '~~/server/mocks/organization-dependency.mock'; +import { allMetrics, commits } from '~~/server/mocks/organization-dependency.mock' as { + allMetrics: OrganizationDependency, + commits: OrganizationDependency +};frontend/app/components/shared/types/contributors.types.ts (2)
53-56: Add JSDoc documentation for the Dependency interface.Document the purpose and constraints of the interface fields.
+/** + * Represents a dependency metric with count and percentage. + * @interface Dependency + * @property {number} count - The total count of items in this dependency category + * @property {number} percentage - The percentage value (0-100) of items in this category + */ export interface Dependency { count: number; percentage: number; }
58-62: Add JSDoc documentation for ContributorDependency and OrganizationDependency interfaces.Document the purpose and relationship between these interfaces.
+/** + * Represents dependency metrics for contributors. + * @interface ContributorDependency + * @property {Dependency} topContributors - Metrics for top contributors + * @property {Dependency} otherContributors - Metrics for remaining contributors + * @property {Contributor[]} list - Detailed list of contributors with their metrics + */ export interface ContributorDependency { topContributors: Dependency; otherContributors: Dependency; list: Contributor[]; } +/** + * Represents dependency metrics for organizations. + * @interface OrganizationDependency + * @property {Dependency} topOrganizations - Metrics for top organizations + * @property {Dependency} otherOrganizations - Metrics for remaining organizations + * @property {Organization[]} list - Detailed list of organizations with their metrics + */ export interface OrganizationDependency { topOrganizations: Dependency; otherOrganizations: Dependency; list: Organization[]; }Also applies to: 64-68
frontend/app/components/modules/project/components/contributors/fragments/organizations-table.vue (1)
2-6: Add ARIA roles to table structure.Enhance accessibility by adding proper ARIA roles to the table structure.
- <div class="lfx-table"> - <div class="lfx-table-header"> + <div class="lfx-table" role="table" aria-label="Organizations contributions"> + <div class="lfx-table-header" role="row"> - <div>Organization</div> - <div>{{ organizationColumnHeader }}</div> + <div role="columnheader">Organization</div> + <div role="columnheader">{{ organizationColumnHeader }}</div> </div>frontend/app/components/modules/project/components/contributors/fragments/dependency-display.vue (1)
44-53: Document color threshold logic and remove unnecessary type casting.The color thresholds seem arbitrary and need documentation. Also, type casting to
ProgressBarTypeis unnecessary when returning string literals that match the type.Apply this diff to improve the code:
-// This needs clarification on how to handle the colors +// Color thresholds for dependency percentage: +// - negative (red): > 80% indicates high dependency risk +// - warning (yellow): > 60% indicates moderate dependency risk +// - positive (green): <= 60% indicates healthy dependency distribution const dependencyColor = computed<ProgressBarType>(() => { if (props.topDependency.percentage > 80) { - return 'negative' as ProgressBarType; + return 'negative'; } if (props.topDependency.percentage > 60) { - return 'warning' as ProgressBarType; + return 'warning'; } - return 'positive' as ProgressBarType; + return 'positive'; });frontend/app/components/modules/project/components/contributors/contributors-leaderboard.vue (2)
17-20: Implement error state UI.The TODO comment indicates missing error state implementation.
Would you like me to help implement an error state component or create an issue to track this task?
58-67: Improve error message for better user experience.The error message could be more user-friendly and provide guidance on potential solutions.
Apply this diff to improve the error message:
showToast( - `Error fetching contributor leaderboard: ${error.value?.statusMessage}`, + `Unable to load contributors data. Please try refreshing the page or contact support if the issue persists.`, ToastTypesEnum.negative, undefined, 10000 );frontend/app/components/modules/project/components/contributors/organizations-leaderboard.vue (1)
16-19: Implement error state handling.The error state is currently incomplete with a TODO comment. Consider implementing proper error handling UI to improve user experience.
Would you like me to help implement the error state component or create an issue to track this task?
frontend/app/components/uikit/progress-bar/progress-bar.scss (1)
1-6: Consider accessibility and responsive design improvements.While the base structure is well-implemented, consider these enhancements:
- Add CSS custom properties for configurable dimensions
- Enforce ARIA role and attributes through class selectors
.c-progress-bar { - @apply flex flex-row items-stretch justify-between gap-0.5 h-2 w-full; + --progress-height: 0.5rem; + @apply flex flex-row items-stretch justify-between gap-0.5 w-full; + height: var(--progress-height); + &[role="progressbar"] { + @apply relative; + } .c-progress-bar__value, .c-progress-bar__empty { @apply rounded-sm transition-all; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
frontend/app/components/modules/project/components/contributors/contributor-dependency.vue(1 hunks)frontend/app/components/modules/project/components/contributors/contributors-leaderboard.vue(3 hunks)frontend/app/components/modules/project/components/contributors/fragments/contributors-table.vue(1 hunks)frontend/app/components/modules/project/components/contributors/fragments/dependency-display.vue(1 hunks)frontend/app/components/modules/project/components/contributors/fragments/metric-dropdown.vue(1 hunks)frontend/app/components/modules/project/components/contributors/fragments/organizations-table.vue(1 hunks)frontend/app/components/modules/project/components/contributors/organization-dependency.vue(1 hunks)frontend/app/components/modules/project/components/contributors/organizations-leaderboard.vue(3 hunks)frontend/app/components/shared/types/contributors.types.ts(2 hunks)frontend/app/components/shared/types/time-periods.ts(1 hunks)frontend/app/components/uikit/avatar-group/avatar-group.scss(1 hunks)frontend/app/components/uikit/index.scss(1 hunks)frontend/app/components/uikit/progress-bar/progress-bar.scss(1 hunks)frontend/app/components/uikit/progress-bar/progress-bar.stories.ts(1 hunks)frontend/app/components/uikit/progress-bar/progress-bar.vue(1 hunks)frontend/app/components/uikit/progress-bar/types/progress-bar.types.ts(1 hunks)frontend/server/api/contributors/contributor-dependency.get.ts(1 hunks)frontend/server/api/contributors/organization-dependency.get.ts(1 hunks)frontend/server/mocks/contributor-dependency.mock.ts(1 hunks)frontend/server/mocks/organization-dependency.mock.ts(1 hunks)frontend/setup/primevue.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/setup/primevue.ts
🔇 Additional comments (16)
frontend/app/components/modules/project/components/contributors/contributor-dependency.vue (4)
3-6: Clearer heading and description look good.The updated heading and descriptive text for Contributor dependency provide helpful context. This improves the user’s understanding of the data shown below.
51-66: Validate route parameters.Using
useRouteis fine, but be sure thatslugandnameare reliably present. Handle cases where they may be undefined, or provide fallback logic within the fetch call.
77-82: Check for potential request caching.
useFetchcould cache responses if configured. If dynamic updates are needed (for example, changing the metric mid-session), confirm that you’re invalidating or refreshing the cache as intended.
109-110: Naming aligns with existing ecosystem.Naming the component
LfxProjectContributorDependencyis consistent with otherLfxcomponents. This fosters a cohesive naming scheme across the codebase.frontend/app/components/uikit/progress-bar/types/progress-bar.types.ts (1)
1-3: Good approach with string literal union.Using
as constand a corresponding type provides strong typing and prevents invalid string assignments. This keeps your progress bar color variants well-scoped.frontend/app/components/shared/types/time-periods.ts (1)
1-11: Versatile time periods for thorough coverage.Defining multiple time ranges (e.g., 90d, custom, etc.) covers common use cases. It’s a handy structure, especially if you plan to introduce dynamic filtering or i18n labels later.
frontend/server/api/contributors/contributor-dependency.get.ts (1)
3-24: LGTM! Well-documented data format.The documentation clearly describes the expected data format and structure.
frontend/app/components/modules/project/components/contributors/fragments/dependency-display.vue (1)
1-24: LGTM! Well-structured and responsive layout.The template has a clean and maintainable structure with good use of semantic HTML and flexbox for responsive design.
frontend/app/components/modules/project/components/contributors/organizations-leaderboard.vue (1)
10-10: LGTM! Good component refactoring.The refactoring to use dedicated
LfxMetricDropdownandLfxOrganizationsTablecomponents improves code modularity and maintainability.Also applies to: 20-24
frontend/app/components/modules/project/components/contributors/organization-dependency.vue (3)
15-18: Implement error state handling.Similar to the organizations leaderboard, the error state needs to be implemented.
22-34: LGTM! Well-structured dependency visualization.The implementation effectively uses
LfxDependencyDisplaywith avatar groups to visualize organization dependencies. The component structure is clean and modular.
86-98: LGTM! Clean data transformation.Good use of computed properties for transforming and filtering data. The implementation is efficient and maintainable.
frontend/app/components/uikit/avatar-group/avatar-group.scss (1)
4-14: LGTM! Improved avatar group styling.The adjustments to margins and outline provide better visual separation and hierarchy. The changes are consistent across different avatar sizes.
frontend/app/components/uikit/index.scss (1)
18-18: LGTM! Added progress bar styles.Clean addition of progress bar styles following the established pattern.
frontend/app/components/uikit/progress-bar/progress-bar.scss (2)
29-31: LGTM!The empty state styling is clean and effective, using flex grow for dynamic sizing and a neutral color that won't compete with the value indicator.
8-27:Details
❓ Verification inconclusive
Verify color contrast ratios and enhance visual feedback.
The state modifiers use semantic colors, but consider:
- Verifying color contrast ratios for accessibility
- Adding hover/focus states if the progress bar is interactive
🌐 Web query:
What are the WCAG 2.1 color contrast requirements for progress indicators?💡 Result:
WCAG 2.1 color contrast requirements for progress indicators primarily fall under Success Criterion 1.4.11 Non-text Contrast (Level AA)[1][6][8]. Here’s a breakdown:
Non-Text Elements
- Progress bars/indicators (e.g., graphical segments or icons) must have a minimum contrast ratio of 3:1 against adjacent colors[1][6][8].
- Example: A loading bar’s filled vs. unfilled sections must meet this ratio to ensure visibility[8].
Text Elements
- Text labels within progress indicators (e.g., “Step 2 of 5”) must follow 1.4.3 Contrast (Minimum):
- Normal text: ≥4.5:1 contrast[1][9].
- Large text (18pt+ or 14pt bold+): ≥3:1[1][9].
Additional Requirements
- State changes (e.g., completed steps marked with checkmarks) must maintain sufficient contrast (3:1) for their visual indicators[6].
- Color reliance: Avoid using color alone to convey progress status (e.g., pair color with text labels or patterns) to comply with 1.4.1 Use of Color[4][6].
Key Exceptions
- Decorative elements without functional meaning are exempt[5].
- Inactive components (e.g., steps not yet available) have no contrast requirement[6].
For compliance, use tools like WebAIM’s Contrast Checker[7] to verify ratios between foreground and background colors in all states of your progress indicator design[4].
Citations:
- 1: https://webaim.org/articles/contrast/
- 2: https://www.washington.edu/accesstech/checklist/contrast/
- 3: https://www.w3.org/WAI/standards-guidelines/wcag/new-in-21/
- 4: https://www.levelaccess.com/blog/clearing-up-a-gray-area-how-a-color-contrast-checker-can-improve-web-accessibility/
- 5: https://www.a11y-collective.com/glossary/colour-contrast/
- 6: https://www.w3.org/WAI/WCAG21/Understanding/non-text-contrast.html
- 7: https://webaim.org/resources/contrastchecker/
- 8: https://mn.gov/mnit/media/blog/?id=38-534351
- 9: https://www.w3.org/WAI/WCAG22/Understanding/contrast-minimum.html
- 10: https://www.lightningdesignsystem.com/guidelines/color/color-accessibility/
Action Required: Enhance Visual Feedback & Verify Contrast
The state modifiers use semantic Tailwind colors effectively. However, please consider the following improvements:
- Contrast Verification: Ensure that the colors applied to the progress bar (filled vs. unfilled or adjacent areas) meet the WCAG 2.1 Non-Text Contrast requirement of at least a 3:1 ratio.
- Interactive States: If the progress bar is intended to be interactive, add hover and focus state styles to provide clear visual feedback for mouse and keyboard interactions.
| <template> | ||
| <!-- This component might change based on Nuno's feedback --> | ||
| <div :class="`c-progress-bar c-progress-bar--${props.color}`"> | ||
| <div class="c-progress-bar__value" :style="{ width: `${props.value}%` }" /> | ||
| <div class="c-progress-bar__empty" /> | ||
| </div> | ||
| </template> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add ARIA attributes for accessibility.
The progress bar should include ARIA attributes to make it accessible to screen readers.
- <div :class="`c-progress-bar c-progress-bar--${props.color}`">
+ <div
+ :class="`c-progress-bar c-progress-bar--${props.color}`"
+ role="progressbar"
+ :aria-valuenow="props.value"
+ aria-valuemin="0"
+ aria-valuemax="100">📝 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.
| <template> | |
| <!-- This component might change based on Nuno's feedback --> | |
| <div :class="`c-progress-bar c-progress-bar--${props.color}`"> | |
| <div class="c-progress-bar__value" :style="{ width: `${props.value}%` }" /> | |
| <div class="c-progress-bar__empty" /> | |
| </div> | |
| </template> | |
| <template> | |
| <!-- This component might change based on Nuno's feedback --> | |
| <div | |
| :class="`c-progress-bar c-progress-bar--${props.color}`" | |
| role="progressbar" | |
| :aria-valuenow="props.value" | |
| aria-valuemin="0" | |
| aria-valuemax="100"> | |
| <div class="c-progress-bar__value" :style="{ width: `${props.value}%` }" /> | |
| <div class="c-progress-bar__empty" /> | |
| </div> | |
| </template> |
| <script setup lang="ts"> | ||
| import type { ProgressBarType } from './types/progress-bar.types'; | ||
|
|
||
| const props = withDefaults( | ||
| defineProps<{ | ||
| value: number; | ||
| // TODO: change this once we have the correct types | ||
| color?: ProgressBarType; | ||
| }>(), | ||
| { | ||
| color: 'normal' | ||
| } | ||
| ); | ||
| </script> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add prop validation and address the TODO comment.
The value prop should include validation to ensure it's between 0 and 100. Also, the TODO comment about types should be addressed.
const props = withDefaults(
defineProps<{
- value: number;
- // TODO: change this once we have the correct types
+ value: number & { __type: 'between 0 and 100' };
color?: ProgressBarType;
}>(),
{
color: 'normal'
}
);
+// Runtime validation for value prop
+watch(() => props.value, (newValue) => {
+ if (newValue < 0 || newValue > 100) {
+ console.warn('Progress bar value must be between 0 and 100');
+ }
+});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <script setup lang="ts"> | |
| import type { ProgressBarType } from './types/progress-bar.types'; | |
| const props = withDefaults( | |
| defineProps<{ | |
| value: number; | |
| // TODO: change this once we have the correct types | |
| color?: ProgressBarType; | |
| }>(), | |
| { | |
| color: 'normal' | |
| } | |
| ); | |
| </script> | |
| <script setup lang="ts"> | |
| import type { ProgressBarType } from './types/progress-bar.types'; | |
| const props = withDefaults( | |
| defineProps<{ | |
| value: number & { __type: 'between 0 and 100' }; | |
| color?: ProgressBarType; | |
| }>(), | |
| { | |
| color: 'normal' | |
| } | |
| ); | |
| // Runtime validation for value prop | |
| watch(() => props.value, (newValue) => { | |
| if (newValue < 0 || newValue > 100) { | |
| console.warn('Progress bar value must be between 0 and 100'); | |
| } | |
| }); | |
| </script> |
| <template> | ||
| <div class="flex flex-row gap-4 items-center mb-6"> | ||
| <lfx-dropdown | ||
| v-model="metric" | ||
| icon="fa-light fa-display-code" | ||
| :options="metricOptions" | ||
| full-width | ||
| center /> | ||
| </div> | ||
| </template> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add accessibility attributes to the dropdown.
The dropdown should include ARIA attributes and a label for screen readers.
- <div class="flex flex-row gap-4 items-center mb-6">
+ <div class="flex flex-row gap-4 items-center mb-6" role="group" aria-label="Metric Selection">
<lfx-dropdown
v-model="metric"
icon="fa-light fa-display-code"
:options="metricOptions"
+ aria-label="Select metric"
full-width
center />
</div>📝 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.
| <template> | |
| <div class="flex flex-row gap-4 items-center mb-6"> | |
| <lfx-dropdown | |
| v-model="metric" | |
| icon="fa-light fa-display-code" | |
| :options="metricOptions" | |
| full-width | |
| center /> | |
| </div> | |
| </template> | |
| <template> | |
| <div class="flex flex-row gap-4 items-center mb-6" role="group" aria-label="Metric Selection"> | |
| <lfx-dropdown | |
| v-model="metric" | |
| icon="fa-light fa-display-code" | |
| :options="metricOptions" | |
| aria-label="Select metric" | |
| full-width | |
| center /> | |
| </div> | |
| </template> |
| export default defineEventHandler(async (event) => { | ||
| const query = getQuery(event); | ||
| const { metric } = query; | ||
|
|
||
| if (metric === 'all') { | ||
| return allMetrics; | ||
| } | ||
|
|
||
| return commits; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and query parameter validation.
The endpoint should validate query parameters and handle potential errors.
export default defineEventHandler(async (event) => {
const query = getQuery(event);
- const { metric } = query;
+ const { metric, project, repository, 'time-period': timePeriod } = query;
+
+ // Validate required parameters
+ if (!metric || !project) {
+ throw createError({
+ statusCode: 400,
+ message: 'Missing required parameters: metric and project'
+ });
+ }
+
+ // Validate metric value
+ const validMetrics = ['all', 'commits']; // Add other valid metrics
+ if (!validMetrics.includes(metric as string)) {
+ throw createError({
+ statusCode: 400,
+ message: `Invalid metric. Must be one of: ${validMetrics.join(', ')}`
+ });
+ }
if (metric === 'all') {
return allMetrics;
}
return commits;
});📝 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.
| export default defineEventHandler(async (event) => { | |
| const query = getQuery(event); | |
| const { metric } = query; | |
| if (metric === 'all') { | |
| return allMetrics; | |
| } | |
| return commits; | |
| }); | |
| export default defineEventHandler(async (event) => { | |
| const query = getQuery(event); | |
| const { metric, project, repository, 'time-period': timePeriod } = query; | |
| // Validate required parameters | |
| if (!metric || !project) { | |
| throw createError({ | |
| statusCode: 400, | |
| message: 'Missing required parameters: metric and project' | |
| }); | |
| } | |
| // Validate metric value | |
| const validMetrics = ['all', 'commits']; // Add other valid metrics if needed | |
| if (!validMetrics.includes(metric as string)) { | |
| throw createError({ | |
| statusCode: 400, | |
| message: `Invalid metric. Must be one of: ${validMetrics.join(', ')}` | |
| }); | |
| } | |
| if (metric === 'all') { | |
| return allMetrics; | |
| } | |
| return commits; | |
| }); |
| export default defineEventHandler(async (event) => { | ||
| const query = getQuery(event); | ||
| const { metric } = query; | ||
|
|
||
| if (metric === 'all') { | ||
| return allMetrics; | ||
| } | ||
|
|
||
| return commits; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and input validation.
The endpoint needs additional error handling and validation:
- Validate required query parameters (project, repository, time-period)
- Handle invalid metric values
- Add type safety for the response data
Consider this implementation:
export default defineEventHandler(async (event) => {
const query = getQuery(event);
- const { metric } = query;
+ const { metric, project, repository, timePeriod } = query;
+
+ // Validate required parameters
+ if (!project || !repository || !timePeriod) {
+ throw createError({
+ statusCode: 400,
+ message: 'Missing required query parameters: project, repository, timePeriod'
+ });
+ }
+
+ // Validate metric value
+ const validMetrics = ['all', 'commits'];
+ if (metric && !validMetrics.includes(metric as string)) {
+ throw createError({
+ statusCode: 400,
+ message: `Invalid metric. Must be one of: ${validMetrics.join(', ')}`
+ });
+ }
if (metric === 'all') {
return allMetrics;
}
return commits;
});📝 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.
| export default defineEventHandler(async (event) => { | |
| const query = getQuery(event); | |
| const { metric } = query; | |
| if (metric === 'all') { | |
| return allMetrics; | |
| } | |
| return commits; | |
| }); | |
| export default defineEventHandler(async (event) => { | |
| const query = getQuery(event); | |
| const { metric, project, repository, timePeriod } = query; | |
| // Validate required parameters | |
| if (!project || !repository || !timePeriod) { | |
| throw createError({ | |
| statusCode: 400, | |
| message: 'Missing required query parameters: project, repository, timePeriod' | |
| }); | |
| } | |
| // Validate metric value | |
| const validMetrics = ['all', 'commits']; | |
| if (metric && !validMetrics.includes(metric as string)) { | |
| throw createError({ | |
| statusCode: 400, | |
| message: `Invalid metric. Must be one of: ${validMetrics.join(', ')}` | |
| }); | |
| } | |
| if (metric === 'all') { | |
| return allMetrics; | |
| } | |
| return commits; | |
| }); |
| <div v-for="(organization, index) in props.organizations" :key="index" class="lfx-table-row"> | ||
| <div class="flex flex-row gap-3 items-center"> | ||
| <lfx-avatar :src="organization.logo" type="organization" /> | ||
| <div>{{ organization.name }}</div> | ||
| </div> | ||
| <div>{{ formatNumber(organization.contributions) }}</div> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve accessibility and list rendering.
- Add ARIA labels for better accessibility
- Use unique identifier instead of index as key
- Add empty state handling
- <div v-for="(organization, index) in props.organizations" :key="index" class="lfx-table-row">
+ <div
+ v-for="organization in props.organizations"
+ :key="organization.name"
+ class="lfx-table-row"
+ role="row"
+ :aria-label="`Organization ${organization.name} with ${organization.contributions} contributions`"
+ >
<div class="flex flex-row gap-3 items-center">
- <lfx-avatar :src="organization.logo" type="organization" />
+ <lfx-avatar :src="organization.logo" type="organization" :alt="`${organization.name} logo`" />
<div>{{ organization.name }}</div>
</div>
<div>{{ formatNumber(organization.contributions) }}</div>
</div>
+ <div v-if="!props.organizations.length" class="p-4 text-center text-gray-500">
+ No organizations found
+ </div>📝 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.
| <div v-for="(organization, index) in props.organizations" :key="index" class="lfx-table-row"> | |
| <div class="flex flex-row gap-3 items-center"> | |
| <lfx-avatar :src="organization.logo" type="organization" /> | |
| <div>{{ organization.name }}</div> | |
| </div> | |
| <div>{{ formatNumber(organization.contributions) }}</div> | |
| </div> | |
| <div | |
| v-for="organization in props.organizations" | |
| :key="organization.name" | |
| class="lfx-table-row" | |
| role="row" | |
| :aria-label="`Organization ${organization.name} with ${organization.contributions} contributions`" | |
| > | |
| <div class="flex flex-row gap-3 items-center"> | |
| <lfx-avatar :src="organization.logo" type="organization" :alt="`${organization.name} logo`" /> | |
| <div>{{ organization.name }}</div> | |
| </div> | |
| <div>{{ formatNumber(organization.contributions) }}</div> | |
| </div> | |
| <div v-if="!props.organizations.length" class="p-4 text-center text-gray-500"> | |
| No organizations found | |
| </div> |
| <template> | ||
| <div class="lfx-table"> | ||
| <div class="lfx-table-header"> | ||
| <div>Contributor</div> | ||
| <div>{{ contributionColumnHeader }}</div> | ||
| </div> | ||
|
|
||
| <div v-for="(contributor, index) in props.contributors" :key="index" class="lfx-table-row"> | ||
| <div class="flex flex-row gap-3 items-center"> | ||
| <lfx-avatar :src="contributor.avatar" type="member" /> | ||
| <div>{{ contributor.name }}</div> | ||
| </div> | ||
| <div> | ||
| {{ formatNumber(contributor.contributions) }} | ||
| <span v-if="props.showPercentage"> ({{ contributor.percentage }}%) </span> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </template> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider creating a base table component.
There's significant code duplication between this component and organizations-table.vue. Consider extracting common functionality into a base table component.
Create a new base component BaseTable.vue:
<template>
<div class="lfx-table" role="table" :aria-label="tableLabel">
<div class="lfx-table-header" role="row">
<div role="columnheader">{{ entityLabel }}</div>
<div role="columnheader">{{ columnHeader }}</div>
</div>
<div
v-for="item in items"
:key="item.name"
class="lfx-table-row"
role="row"
:aria-label="`${entityLabel} ${item.name} with ${item.contributions} contributions`"
>
<div class="flex flex-row gap-3 items-center">
<lfx-avatar :src="item.avatar || item.logo" :type="avatarType" :alt="`${item.name} ${avatarType === 'member' ? 'avatar' : 'logo'}`" />
<div>{{ item.name }}</div>
</div>
<div>
{{ formatNumber(item.contributions) }}
<slot name="extra-info" :item="item"></slot>
</div>
</div>
<div v-if="!items.length" class="p-4 text-center text-gray-500">
No {{ entityLabel.toLowerCase() }}s found
</div>
</div>
</template>Then update this component to use the base:
<template>
- <div class="lfx-table">
- <!-- ... existing template ... -->
- </div>
+ <base-table
+ entity-label="Contributor"
+ :column-header="contributionColumnHeader"
+ :items="contributors"
+ avatar-type="member"
+ table-label="Contributors contributions"
+ >
+ <template #extra-info="{ item }">
+ <span v-if="showPercentage"> ({{ item.percentage }}%) </span>
+ </template>
+ </base-table>
</template>| export const allMetrics = { | ||
| topContributors: { | ||
| count: 3, | ||
| percentage: 68 | ||
| }, | ||
| otherContributors: { | ||
| count: 1000, | ||
| percentage: 32 | ||
| }, | ||
| list: [ | ||
| { | ||
| avatar: 'https://i.pravatar.cc/150?u=john.doe@example.com', | ||
| name: 'John Doe', | ||
| contributions: 1000, | ||
| percentage: 20, | ||
| email: 'john.doe@example.com' | ||
| }, | ||
| { | ||
| avatar: 'https://i.pravatar.cc/150?u=jane.doe@example.com', | ||
| name: 'Jane Doe', | ||
| contributions: 900, | ||
| percentage: 15, | ||
| email: 'jane.doe@example.com' | ||
| }, | ||
| { | ||
| avatar: 'https://i.pravatar.cc/150?u=john.smith@example.com', | ||
| name: 'John Smith', | ||
| contributions: 800, | ||
| percentage: 10, | ||
| email: 'john.smith@example.com' | ||
| }, | ||
| { | ||
| avatar: 'https://i.pravatar.cc/150?u=jane.smith@example.com', | ||
| name: 'Jane Smith', | ||
| contributions: 700, | ||
| percentage: 8, | ||
| email: 'jane.smith@example.com' | ||
| }, | ||
| { | ||
| avatar: 'https://i.pravatar.cc/150?u=jim.smith@example.com', | ||
| name: 'Tom Ford', | ||
| contributions: 600, | ||
| percentage: 5, | ||
| email: 'tom.ford@example.com' | ||
| } | ||
| ] | ||
| }; |
There was a problem hiding this comment.
Fix data consistency in mock percentages.
The individual contributor percentages (20% + 15% + 10% + 8% + 5% = 58%) don't sum up to the topContributors.percentage (68%).
Also, using an external service (pravatar.cc) for mock avatars could make tests unreliable.
Consider using local placeholder images or base64 encoded images for more reliable tests.
| export const allMetrics = { | ||
| topOrganizations: { | ||
| count: 2, | ||
| percentage: 82 | ||
| }, | ||
| otherOrganizations: { | ||
| count: 100, | ||
| percentage: 18 | ||
| }, | ||
| list: [ | ||
| { | ||
| logo: 'https://banner2.cleanpng.com/20240110/flp/transparent-google-logo-colorful-google-logo-on-black-1710930944198.webp', | ||
| name: 'Google', | ||
| contributions: 10000, | ||
| website: 'https://www.google.com' | ||
| }, | ||
| { | ||
| logo: 'https://e7.pngegg.com/pngimages/782/185/png-clipart-desktop-apple-logo-apple-logo-computer-logo.png', | ||
| name: 'Apple', | ||
| contributions: 9000, | ||
| website: 'https://www.apple.com' | ||
| }, | ||
| { | ||
| logo: 'https://cdn-icons-png.flaticon.com/256/732/732221.png', | ||
| name: 'Microsoft', | ||
| contributions: 8000, | ||
| website: 'https://www.microsoft.com' | ||
| }, | ||
| { | ||
| logo: 'https://i.pinimg.com/originals/01/ca/da/01cada77a0a7d326d85b7969fe26a728.jpg', | ||
| name: 'Amazon', | ||
| contributions: 7000, | ||
| website: 'https://www.amazon.com' | ||
| }, | ||
| { | ||
| logo: 'https://i.pinimg.com/474x/f7/99/20/f79920f4cb34986684e29df42ec0cebe.jpg', | ||
| name: 'Facebook', | ||
| contributions: 6000, | ||
| website: 'https://www.facebook.com' | ||
| } | ||
| ] | ||
| }; | ||
|
|
||
| export const commits = { | ||
| topOrganizations: { | ||
| count: 3, | ||
| percentage: 73 | ||
| }, | ||
| otherOrganizations: { | ||
| count: 99, | ||
| percentage: 27 | ||
| }, | ||
| list: [ | ||
| { | ||
| logo: 'https://e7.pngegg.com/pngimages/708/311/png-clipart-twitter-twitter-thumbnail.png', | ||
| name: 'Twitter', | ||
| contributions: 5000, | ||
| website: 'https://www.twitter.com' | ||
| }, | ||
| { | ||
| logo: 'https://www.citypng.com/public/uploads/preview/square-instagram-logo-photos-social-media-701751694793082uj6ej5fkfx.png', | ||
| name: 'Instagram', | ||
| contributions: 4000, | ||
| website: 'https://www.instagram.com' | ||
| }, | ||
| { | ||
| logo: 'https://e7.pngegg.com/pngimages/1022/248/png-clipart-tesla-logo-car-logo-tesla-icons-logos-emojis-car-logos-thumbnail.png', | ||
| name: 'Tesla', | ||
| contributions: 3000, | ||
| website: 'https://www.tesla.com' | ||
| }, | ||
| { | ||
| logo: 'https://encrypted-tbn0.gstatic.com/images?q=tbn:ANd9GcSFf8io--ELan4yZyqWVpDWOckW2IzHD8Cfwg&s', | ||
| name: 'Netflix', | ||
| contributions: 2000, | ||
| website: 'https://www.netflix.com' | ||
| }, | ||
| { | ||
| logo: 'https://i.pinimg.com/originals/e9/da/0c/e9da0c83b0a7ec866e17c100079c9d88.jpg', | ||
| name: 'WhatsApp', | ||
| contributions: 1000, | ||
| website: 'https://www.whatsapp.com' | ||
| } | ||
| ] | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace external image URLs with local assets.
Using external image URLs (especially Google-cached ones) in mock data could make tests unreliable as the URLs might expire or be blocked.
Consider:
- Using local placeholder images in the project's assets directory
- Using base64 encoded images for mock data
- Creating a dedicated mock assets directory for test data
2 participants
In this PR:
Future work
Need to wire this to actual backend endpoint when it's ready
Require adjustments on UI particularly on empty states and chart hover interactions (when designs are ready)
Tickets:
INS-66
INS-67
Summary by CodeRabbit