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>
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>
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 PR updates several Vue components, TypeScript types, and utility functions. It corrects a title typo and improves layout in the Average Time to Merge component while adding data fetching, computed properties, and error handling. Other components have minor formatting improvements, identifier renames, and enhanced import readability. New functionalities include a duration formatter, additional chart grid configurations, extended delta display options, and a new API endpoint with accompanying mock data for average merge times. Changes
Sequence Diagram(s)sequenceDiagram
participant C as AverageTimeToMerge.vue
participant F as useFetch
participant A as API Endpoint
participant V as View (Chart/Delta Display)
C->>F: Invoke API call with timePeriod and project params
F->>A: Request average merge data
A-->>F: Return mock average merge data (or error)
F-->>C: Provide fetched data to component
C->>C: Process data into computed properties (average, summary, chartData)
alt Data Pending
C->>V: Render spinner
else Data Success
C->>V: Render chart with data and delta display
end
Possibly Related PRs
Suggested Reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (5)
🪧 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: 3
🧹 Nitpick comments (13)
frontend/app/components/modules/project/components/development/types/average-time-merge.types.ts (1)
1-10: Clean interface implementation for the new average time to merge feature.The
AverageTimeMergeinterface properly structures the data needed for the new widget, with a summary section and time-based data array. This aligns well with the PR objective to add an average time to merge widget.Consider adding JSDoc comments to document the expected date format for the
dateFromanddateTostring properties to help other developers understand the expected format.frontend/server/mocks/merge-lead-time.mock.ts (1)
11-30: Consider standardizing time units across metrics for better comparability.The data uses mixed time units (hours for pickup but days for other metrics). Consider standardizing to a single unit or ensuring the UI handles this difference appropriately to avoid confusion for users comparing these metrics.
frontend/server/api/projects/development/review-time-by-pr-size.get.ts (1)
15-21: Consider implementing query parameter handling.Currently, the handler ignores the query parameters mentioned in the comments. Even in a mock implementation, it would be helpful to at least log or validate these parameters to better simulate the real API behavior.
-export default defineEventHandler(async () => reviewTimeByPr); +export default defineEventHandler(async (event) => { + const query = getQuery(event); + // Log or validate query parameters + console.log('Query parameters:', query); + + // In the future, you could filter or transform data based on these parameters + return reviewTimeByPr; +});frontend/app/components/modules/project/components/development/contributions-outside-work-hours.vue (1)
119-120: Fix error message for clarityThe error message references "social mentions" but this component is for "contributions outside work hours".
- `Error fetching social mentions: ${error.value?.statusMessage}`, + `Error fetching contributions outside work hours: ${error.value?.statusMessage}`,frontend/server/api/projects/development/merge-lead-time.get.ts (1)
1-44: Well-documented API endpoint for merge lead time metrics.The API endpoint is properly structured with clear documentation explaining both the expected data format and query parameters. The implementation returns mock data for now, which aligns with the PR objective of implementing an average time to merge widget with mock data.
However, I recommend adding error handling and actually using the query parameters even with mock data:
-export default defineEventHandler(async () => mergeLeadTime); +export default defineEventHandler(async (event) => { + const query = getQuery(event); + // Validate required parameters + const { project, repository } = query; + if (!project || !repository) { + throw createError({ + statusCode: 400, + statusMessage: 'Missing required parameters: project and repository' + }); + } + + return mergeLeadTime; +});frontend/app/components/modules/project/components/development/fragments/merge-lead-item.vue (2)
14-18: Consider enhancing the change indicator with more context.The current implementation shows thumbs up/down icons with values, but users might benefit from more context about what makes a change positive or negative.
-<div class="flex gap-2 items-center text-sm h-6 px-2.5 py-1 rounded-full" :class="valueClass"> - <lfx-icon :name="itemValue.changeType === 'positive' ? 'thumbs-up' : 'thumbs-down'" :size="14" /> - {{ itemValue.value }} - {{ itemValue.unit }} -</div> +<div class="flex gap-2 items-center text-sm h-6 px-2.5 py-1 rounded-full" :class="valueClass" :title="changeTooltip"> + <lfx-icon :name="itemValue.changeType === 'positive' ? 'thumbs-up' : 'thumbs-down'" :size="14" /> + {{ itemValue.value }} + {{ itemValue.unit }} +</div>Add this computed property to provide tooltip context:
const changeTooltip = computed(() => { const direction = props.itemValue.changeType === 'positive' ? 'decreased' : 'increased'; return `Time has ${direction} by ${props.itemValue.value} ${props.itemValue.unit}`; });
39-43: Simplify computed property syntax.The computed property definition has an unnecessary comment marker (
//) that should be removed.-const valueClass = computed(// - () => (props.itemValue.changeType === 'positive' - ? 'bg-positive-50 text-positive-600' - : 'bg-negative-50 text-negative-600') -); +const valueClass = computed(() => + props.itemValue.changeType === 'positive' + ? 'bg-positive-50 text-positive-600' + : 'bg-negative-50 text-negative-600' +);frontend/app/components/uikit/progress-bar/progress-bar.vue (2)
5-5: Use a more stable key if possible
Using the array index as a key might lead to suboptimal DOM reusability if array elements are reordered. Consider using a unique ID if available.
20-21: Boolean prop naming
ThehideEmptyprop is correct, but for clarity, consider a positively named prop (e.g.,showEmpty) that defaults totrue. This can improve readability.frontend/app/components/uikit/delta-display/delta-display.vue (1)
45-53: Previous display logic
UsinghidePreviousValueto omit the text is a simple, effective toggle. Consider including a space or separator for the unit if nonempty, but otherwise this is clean.frontend/app/components/modules/project/components/development/review-time-by-pull-request-size.vue (1)
68-77: Error handling
Watchingerrorand showing a toast is a clean approach. The message references “social mentions,” which may be inconsistent with “review time by PR size.” Double-check or update the wording for clarity.frontend/app/components/modules/project/components/development/merge-lead-time.vue (1)
6-9: Consider updating the descriptive text for accuracy.The paragraph describes "Active contributor" activity but doesn't directly elaborate on merge lead time. It might help to refine or remove this text to keep the focus on merge lead time insights.
frontend/app/components/modules/project/components/development/average-time-to-merge.vue (1)
6-9: Confirm relevance of the contributor description.Similar to the merge lead time component, the text references active contributor activities. Consider revising or removing to emphasize how average time to merge is calculated or interpreted.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
frontend/app/components/modules/project/components/development/average-time-to-merge.vue(1 hunks)frontend/app/components/modules/project/components/development/contributions-outside-work-hours.vue(1 hunks)frontend/app/components/modules/project/components/development/fragments/merge-lead-item.vue(1 hunks)frontend/app/components/modules/project/components/development/merge-lead-time.vue(1 hunks)frontend/app/components/modules/project/components/development/review-time-by-pull-request-size.vue(1 hunks)frontend/app/components/modules/project/components/development/types/average-time-merge.types.ts(1 hunks)frontend/app/components/modules/project/components/development/types/merge-lead-time.types.ts(1 hunks)frontend/app/components/modules/project/components/development/types/review-time-by-pr.types.ts(1 hunks)frontend/app/components/modules/project/views/development.vue(2 hunks)frontend/app/components/shared/utils/formatter.ts(2 hunks)frontend/app/components/uikit/chart/configs/bar.chart.ts(1 hunks)frontend/app/components/uikit/chart/helpers/chart-helpers.ts(1 hunks)frontend/app/components/uikit/delta-display/delta-display.vue(2 hunks)frontend/app/components/uikit/delta-display/types/delta-display.types.ts(1 hunks)frontend/app/components/uikit/progress-bar/progress-bar.scss(1 hunks)frontend/app/components/uikit/progress-bar/progress-bar.vue(2 hunks)frontend/server/api/projects/development/average-time-merge.get.ts(1 hunks)frontend/server/api/projects/development/merge-lead-time.get.ts(1 hunks)frontend/server/api/projects/development/review-time-by-pr-size.get.ts(1 hunks)frontend/server/mocks/average-time-merge.mock.ts(1 hunks)frontend/server/mocks/merge-lead-time.mock.ts(1 hunks)frontend/server/mocks/review-time-by-pr.mock.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/server/mocks/review-time-by-pr.mock.ts
🔇 Additional comments (36)
frontend/app/components/modules/project/components/development/types/review-time-by-pr.types.ts (1)
1-7: Well-structured interface design!The
ReviewTimeByPrIteminterface is clearly defined with appropriate types for each property. The interface structure effectively supports the average time to merge widget mentioned in the PR objectives.frontend/app/components/uikit/chart/configs/bar.chart.ts (1)
12-16:Details
❓ Verification inconclusive
Grid padding fix implemented as described in PR objectives.
The added grid configuration addresses the bar chart padding issue mentioned in the PR description. This will improve the visual presentation of chart data by providing consistent spacing.
Make sure this grid configuration doesn't cause any visual issues with the rightmost bars being cut off since you've set
right: 0. Run the following to check other chart implementations that might be affected:
🏁 Script executed:
#!/bin/bash # Find all Vue files that might be using bar charts rg -l "getBarChartConfig" --type vueLength of output: 64
Below is a rewritten review comment that highlights the fix and advises a re-check using an updated search command:
Grid Padding Fix – Please Verify Vue Usage
The new grid configuration fulfills the PR objectives by ensuring consistent spacing. However, since setting
right: 0might clip the rightmost bars if other chart implementations expect extra padding, please verify that no Vue components (or related files) use thegetBarChartConfigfunction in a way that may be affected.To confirm, run this updated command which explicitly searches for
.vuefiles:#!/bin/bash # Find all Vue files that might be using bar charts by pattern matching the .vue extension rg -l "getBarChartConfig" -g "*.vue"If any files are returned, review them to ensure that the grid configuration change does not introduce visual issues.
frontend/app/components/uikit/chart/helpers/chart-helpers.ts (1)
31-32: Useful utility function for chart data processing.The
getMaxValuefunction provides a clean way to find the maximum value in chart data, which will be helpful for setting appropriate scales in the average time to merge charts. Good use of the reduce method to iterate through the data array.frontend/app/components/shared/utils/formatter.ts (2)
8-8: Good addition of the Luxon Duration import.The import for Luxon's Duration class is correctly added to support the new duration formatting functionality.
25-33: Well-implemented duration formatting function with clear documentation.The
formatNumberToDurationfunction is well-documented with JSDoc comments and has a clean implementation that handles different duration formats based on the input value. The conditional formatting (hours+minutes vs minutes only) provides a user-friendly output.frontend/server/mocks/average-time-merge.mock.ts (1)
7-8: Consider using ISO date format consistently.The date format is using ISO 8601 format which is good. However, ensure that date formats are consistent across all components that will consume this data.
frontend/server/mocks/merge-lead-time.mock.ts (1)
1-32: Well-structured mock data with clear organization.The mock data for merge lead time is well-structured with a clear separation between summary metrics and detailed breakdown by stages. The inclusion of units and change type indicators will be helpful for UI rendering.
frontend/server/api/projects/development/review-time-by-pr-size.get.ts (1)
1-21: Well-documented API endpoint with clear data format expectations.The endpoint implementation is clean with good documentation of the expected data format and query parameters. The comment about future time-period values ('90d', '1y', '5y') is helpful for future development.
frontend/app/components/modules/project/components/development/types/merge-lead-time.types.ts (1)
1-16: Well-structured type definitions for merge lead time metricsThe interfaces are well-defined with appropriate types and structure to represent different stages of the merge process. This creates a strong foundation for implementing the average time to merge feature mentioned in the PR objectives.
The
MergeLeadTimeItemcaptures individual metrics with value, unit, and change type, while theMergeLeadTimeinterface organizes these items into a logical workflow structure (pickup, review, accepted, prMerged).frontend/app/components/uikit/delta-display/types/delta-display.types.ts (1)
12-13: Good enhancement to DeltaDisplayPropsThese new optional properties improve the flexibility of the delta display component:
unitallows specifying measurement units (%, days, hours)isDurationenables special handling for time-based valuesThis aligns well with the PR objective of implementing time-based metrics.
frontend/server/api/projects/development/average-time-merge.get.ts (1)
1-27:Details
❓ Verification inconclusive
Well-documented API endpoint for average time to merge
The endpoint is clearly documented with the expected response format and query parameters. This provides good guidance for frontend integration.
One consideration for future implementation:
The current implementation returns mock data without using the documented query parameters (project, repository, time-period). While this is fine for initial development, ensure these parameters are properly handled when implementing the real API integration.
🏁 Script executed:
#!/bin/bash # Verify that the query parameters will be used in future implementation # Check if there are any other API endpoints that properly utilize query parameters # This can serve as a reference for the correct implementation pattern rg -A 5 "defineEventHandler\(async \(\{.*query.*\}\)" --type tsLength of output: 65
API Endpoint for Average Time Merge – Future Enhancement Advisory
The endpoint is well-documented with the expected response format and query parameters. However, note that it currently returns static mock data without processing the documented query parameters (project, repository, time-period). When implementing the real API integration, please ensure that these parameters are correctly handled.
- Future improvements should integrate parameter handling as per the documentation.
- Manually verify that similar API endpoints (if any) process query parameters consistently, since our automated script did not return any matching examples.
frontend/app/components/modules/project/components/development/contributions-outside-work-hours.vue (1)
15-17: Improved display with unit specificationAdding the unit attribute to the delta display component enhances clarity by explicitly indicating the percentage unit. This is a good UI improvement that leverages the new
unitproperty added to theDeltaDisplayPropstype.frontend/app/components/uikit/progress-bar/progress-bar.scss (3)
2-2: Improved progress bar layout with centered items.The flexbox layout has been updated to use
items-centerinstead ofitems-stretch, which will provide better alignment for the progress bar components.
5-6: Height moved from container to individual bar elements.This is a good structural change as it applies the height directly to the bar elements instead of the container, allowing for proper vertical alignment.
8-10: New label styling added for progress bar.The addition of the label class with appropriate styling enhances the progress bar component to support descriptive labels. This aligns with the PR objective to improve UI for time metrics display.
frontend/app/components/modules/project/components/development/fragments/merge-lead-item.vue (2)
1-20: Well-structured timeline item component with clear visual indicators.The component template creates a visually appealing timeline item with icons, connecting lines, and color-coded indicators for positive/negative changes. The layout is responsive and follows good UI practices.
21-44: Clean component definition with appropriate type imports.The component uses Vue 3's Composition API with TypeScript properly. Props are well-defined with appropriate defaults and types.
frontend/app/components/modules/project/views/development.vue (3)
25-25: Updated scroll view ID for consistency.The ID has been updated from "forks-review-time-by-pull-request-size" to "review-time-by-pull-request-size" for better consistency in naming conventions.
31-31: Updated scroll view ID for consistency.The ID has been updated from "forks-wait-time-first-review" to "wait-time-first-review" for better consistency in naming conventions.
76-78: Updated side navigation item labels for consistency.The labels have been updated to match the new ID naming conventions, maintaining consistency throughout the UI.
frontend/app/components/uikit/progress-bar/progress-bar.vue (2)
7-8: Conditional sections look good
The newly introduced conditional label and empty section provide flexibility and clarity while keeping the markup simple.
24-26: Defaults are consistent
Providing default values forcolor,hideEmpty, andlabelensures the component behaves predictably when props are omitted.frontend/app/components/uikit/delta-display/delta-display.vue (6)
2-2: Conditional layout is well-handled
Toggling the layout via theflex-col-reverseclass is straightforward and increases flexibility for different use cases.
8-8: Separation of percentage and delta
Displaying{{ percentage }}%separately fromdeltaDisplaykeeps the layout clean and clear.
10-10: Minor layout update
Changing the styling totext-neutral-400 text-xsfor the “last period” label is a small, targeted improvement that doesn't pose any obvious issues.
20-20: Additional import is fine
ImportingformatNumberToDurationalongsideformatNumberis an appropriate approach to handle both numeric and duration formats without polluting the code.
33-33: Handling signed durations
Prepending the sign to a duration-based value is logical. Double-check negative durations aren’t expected, since the sign is assumed to be plus or minus.
38-43: Computed property for delta display
Wrapping the extra unit in parentheses is a neat way to distinguish it from the percentage. This also prevents multiline complexities in the template.frontend/app/components/modules/project/components/development/review-time-by-pull-request-size.vue (5)
6-6: Clarify descriptive text
This paragraph references “active contributor” but the component deals with “review time by PR size.” Ensure the text is updated or confirmed correct.
10-28: Structured layout for review data
Introducing<hr>and wrapping content in a<section>improves visual separation. The conditionalv-if="status === 'success'"approach is clean, and fallback spinner usage is a best practice.
33-41: Nuxt composables usage
UsinguseFetchanduseRoutekeeps data loading co-located. The imports for card, spinner, and progress bar show good modular design.
42-49: Prop default design
ProvidingtimePeriod: '90d'as a default ensures consistent behavior if no time period is specified. This follows a good pattern for optional props.
50-67: Handling remote data
Storingdata,status, anderrorfromuseFetchis concise and aligns well with Nuxt’s pattern. The computedreviewTimeByPrandmaxValuesteps are well-structured for chart calculations.frontend/app/components/modules/project/components/development/merge-lead-time.vue (1)
60-68: Ensure route parameters are always valid.Since
route.params.slugandroute.params.nameare used to build the API URL, consider implementing safeguards or default values if they are missing or invalid to prevent potential fetch errors.frontend/app/components/modules/project/components/development/average-time-to-merge.vue (2)
4-4: Use of the updated heading looks good.Renaming to "Average time to merge" better aligns with this component's purpose.
85-98: Validate Y-axis max calculation.You're adding 20 to
Math.ceil(value.max / 20) * 20. Ensure that this extra padding truly benefits readability. In some small datasets, it might leave excessive empty space above the highest bar.
| export const averageTimeMerge = { | ||
| summary: { | ||
| current: 3632, // in minutes | ||
| previous: 3608, // in minutes | ||
| percentageChange: 2.3, | ||
| changeValue: 24, | ||
| periodFrom: '2024-11-11T00:00:00Z', | ||
| periodTo: '2025-02-12T00:00:00Z' | ||
| }, | ||
| data: [ | ||
| { | ||
| dateFrom: '2024-11-11T00:00:00Z', | ||
| dateTo: '2024-11-18T00:00:00Z', | ||
| averageTime: 89 // in hours | ||
| }, | ||
| { | ||
| dateFrom: '2024-11-18T00:00:00Z', | ||
| dateTo: '2024-11-25T00:00:00Z', | ||
| averageTime: 60 | ||
| }, | ||
| { | ||
| dateFrom: '2024-11-25T00:00:00Z', | ||
| dateTo: '2024-12-02T00:00:00Z', | ||
| averageTime: 88 | ||
| }, | ||
| { | ||
| dateFrom: '2024-12-02T00:00:00Z', | ||
| dateTo: '2024-12-09T00:00:00Z', | ||
| averageTime: 51 | ||
| }, | ||
| { | ||
| dateFrom: '2024-12-09T00:00:00Z', | ||
| dateTo: '2024-12-16T00:00:00Z', | ||
| averageTime: 89 | ||
| }, | ||
| { | ||
| dateFrom: '2024-12-16T00:00:00Z', | ||
| dateTo: '2024-12-23T00:00:00Z', | ||
| averageTime: 52 | ||
| }, | ||
| { | ||
| dateFrom: '2024-12-23T00:00:00Z', | ||
| dateTo: '2025-01-06T00:00:00Z', | ||
| averageTime: 90 | ||
| }, | ||
| { | ||
| dateFrom: '2024-12-23T00:00:00Z', | ||
| dateTo: '2025-01-06T00:00:00Z', | ||
| averageTime: 73 | ||
| }, | ||
| { | ||
| dateFrom: '2025-01-06T00:00:00Z', | ||
| dateTo: '2025-01-13T00:00:00Z', | ||
| averageTime: 51 | ||
| }, | ||
| { | ||
| dateFrom: '2025-01-13T00:00:00Z', | ||
| dateTo: '2025-01-20T00:00:00Z', | ||
| averageTime: 94 | ||
| }, | ||
| { | ||
| dateFrom: '2025-01-20T00:00:00Z', | ||
| dateTo: '2025-01-27T00:00:00Z', | ||
| averageTime: 92 | ||
| }, | ||
| { | ||
| dateFrom: '2025-01-27T00:00:00Z', | ||
| dateTo: '2025-02-03T00:00:00Z', | ||
| averageTime: 45 | ||
| }, | ||
| { | ||
| dateFrom: '2025-02-03T00:00:00Z', | ||
| dateTo: '2025-02-10T00:00:00Z', | ||
| averageTime: 33 | ||
| }, | ||
| { | ||
| dateFrom: '2025-02-10T00:00:00Z', | ||
| dateTo: '2025-02-17T00:00:00Z', | ||
| averageTime: 66 | ||
| }, | ||
| { | ||
| dateFrom: '2025-02-17T00:00:00Z', | ||
| dateTo: '2025-02-24T00:00:00Z', | ||
| averageTime: 44 | ||
| }, | ||
| { | ||
| dateFrom: '2025-02-24T00:00:00Z', | ||
| dateTo: '2025-03-03T00:00:00Z', | ||
| averageTime: 97 | ||
| } | ||
| ] | ||
| }; |
There was a problem hiding this comment.
Fix inconsistent time units and duplicate date range in mock data.
There are two issues in this mock data:
-
Time units inconsistency: The summary uses minutes (lines 3-4) while the data array uses hours (line 14 onwards). This could cause confusion when consuming this data.
-
Duplicate date range: Entries at lines 41-45 and 46-50 have identical date ranges (Dec 23, 2024 to Jan 6, 2025) but different average times (90 and 73 hours).
export const averageTimeMerge = {
summary: {
current: 3632, // in minutes
previous: 3608, // in minutes
percentageChange: 2.3,
changeValue: 24,
periodFrom: '2024-11-11T00:00:00Z',
periodTo: '2025-02-12T00:00:00Z'
},
data: [
{
dateFrom: '2024-11-11T00:00:00Z',
dateTo: '2024-11-18T00:00:00Z',
- averageTime: 89 // in hours
+ averageTime: 89, // in hours
+ unit: 'hours'
},
// Update all other data entries to include unit
// ...
- {
- dateFrom: '2024-12-23T00:00:00Z',
- dateTo: '2025-01-06T00:00:00Z',
- averageTime: 90
- },
{
dateFrom: '2024-12-23T00:00:00Z',
dateTo: '2025-01-06T00:00:00Z',
- averageTime: 73
+ averageTime: 73
},
// ...📝 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 const averageTimeMerge = { | |
| summary: { | |
| current: 3632, // in minutes | |
| previous: 3608, // in minutes | |
| percentageChange: 2.3, | |
| changeValue: 24, | |
| periodFrom: '2024-11-11T00:00:00Z', | |
| periodTo: '2025-02-12T00:00:00Z' | |
| }, | |
| data: [ | |
| { | |
| dateFrom: '2024-11-11T00:00:00Z', | |
| dateTo: '2024-11-18T00:00:00Z', | |
| averageTime: 89 // in hours | |
| }, | |
| { | |
| dateFrom: '2024-11-18T00:00:00Z', | |
| dateTo: '2024-11-25T00:00:00Z', | |
| averageTime: 60 | |
| }, | |
| { | |
| dateFrom: '2024-11-25T00:00:00Z', | |
| dateTo: '2024-12-02T00:00:00Z', | |
| averageTime: 88 | |
| }, | |
| { | |
| dateFrom: '2024-12-02T00:00:00Z', | |
| dateTo: '2024-12-09T00:00:00Z', | |
| averageTime: 51 | |
| }, | |
| { | |
| dateFrom: '2024-12-09T00:00:00Z', | |
| dateTo: '2024-12-16T00:00:00Z', | |
| averageTime: 89 | |
| }, | |
| { | |
| dateFrom: '2024-12-16T00:00:00Z', | |
| dateTo: '2024-12-23T00:00:00Z', | |
| averageTime: 52 | |
| }, | |
| { | |
| dateFrom: '2024-12-23T00:00:00Z', | |
| dateTo: '2025-01-06T00:00:00Z', | |
| averageTime: 90 | |
| }, | |
| { | |
| dateFrom: '2024-12-23T00:00:00Z', | |
| dateTo: '2025-01-06T00:00:00Z', | |
| averageTime: 73 | |
| }, | |
| { | |
| dateFrom: '2025-01-06T00:00:00Z', | |
| dateTo: '2025-01-13T00:00:00Z', | |
| averageTime: 51 | |
| }, | |
| { | |
| dateFrom: '2025-01-13T00:00:00Z', | |
| dateTo: '2025-01-20T00:00:00Z', | |
| averageTime: 94 | |
| }, | |
| { | |
| dateFrom: '2025-01-20T00:00:00Z', | |
| dateTo: '2025-01-27T00:00:00Z', | |
| averageTime: 92 | |
| }, | |
| { | |
| dateFrom: '2025-01-27T00:00:00Z', | |
| dateTo: '2025-02-03T00:00:00Z', | |
| averageTime: 45 | |
| }, | |
| { | |
| dateFrom: '2025-02-03T00:00:00Z', | |
| dateTo: '2025-02-10T00:00:00Z', | |
| averageTime: 33 | |
| }, | |
| { | |
| dateFrom: '2025-02-10T00:00:00Z', | |
| dateTo: '2025-02-17T00:00:00Z', | |
| averageTime: 66 | |
| }, | |
| { | |
| dateFrom: '2025-02-17T00:00:00Z', | |
| dateTo: '2025-02-24T00:00:00Z', | |
| averageTime: 44 | |
| }, | |
| { | |
| dateFrom: '2025-02-24T00:00:00Z', | |
| dateTo: '2025-03-03T00:00:00Z', | |
| averageTime: 97 | |
| } | |
| ] | |
| }; | |
| export const averageTimeMerge = { | |
| summary: { | |
| current: 3632, // in minutes | |
| previous: 3608, // in minutes | |
| percentageChange: 2.3, | |
| changeValue: 24, | |
| periodFrom: '2024-11-11T00:00:00Z', | |
| periodTo: '2025-02-12T00:00:00Z' | |
| }, | |
| data: [ | |
| { | |
| dateFrom: '2024-11-11T00:00:00Z', | |
| dateTo: '2024-11-18T00:00:00Z', | |
| averageTime: 89, // in hours | |
| unit: 'hours' | |
| }, | |
| { | |
| dateFrom: '2024-11-18T00:00:00Z', | |
| dateTo: '2024-11-25T00:00:00Z', | |
| averageTime: 60, | |
| unit: 'hours' | |
| }, | |
| { | |
| dateFrom: '2024-11-25T00:00:00Z', | |
| dateTo: '2024-12-02T00:00:00Z', | |
| averageTime: 88, | |
| unit: 'hours' | |
| }, | |
| { | |
| dateFrom: '2024-12-02T00:00:00Z', | |
| dateTo: '2024-12-09T00:00:00Z', | |
| averageTime: 51, | |
| unit: 'hours' | |
| }, | |
| { | |
| dateFrom: '2024-12-09T00:00:00Z', | |
| dateTo: '2024-12-16T00:00:00Z', | |
| averageTime: 89, | |
| unit: 'hours' | |
| }, | |
| { | |
| dateFrom: '2024-12-16T00:00:00Z', | |
| dateTo: '2024-12-23T00:00:00Z', | |
| averageTime: 52, | |
| unit: 'hours' | |
| }, | |
| // Removed duplicate entry with date range Dec 23, 2024 to Jan 6, 2025 (the one with averageTime 90) | |
| { | |
| dateFrom: '2024-12-23T00:00:00Z', | |
| dateTo: '2025-01-06T00:00:00Z', | |
| averageTime: 73, | |
| unit: 'hours' | |
| }, | |
| { | |
| dateFrom: '2025-01-06T00:00:00Z', | |
| dateTo: '2025-01-13T00:00:00Z', | |
| averageTime: 51, | |
| unit: 'hours' | |
| }, | |
| { | |
| dateFrom: '2025-01-13T00:00:00Z', | |
| dateTo: '2025-01-20T00:00:00Z', | |
| averageTime: 94, | |
| unit: 'hours' | |
| }, | |
| { | |
| dateFrom: '2025-01-20T00:00:00Z', | |
| dateTo: '2025-01-27T00:00:00Z', | |
| averageTime: 92, | |
| unit: 'hours' | |
| }, | |
| { | |
| dateFrom: '2025-01-27T00:00:00Z', | |
| dateTo: '2025-02-03T00:00:00Z', | |
| averageTime: 45, | |
| unit: 'hours' | |
| }, | |
| { | |
| dateFrom: '2025-02-03T00:00:00Z', | |
| dateTo: '2025-02-10T00:00:00Z', | |
| averageTime: 33, | |
| unit: 'hours' | |
| }, | |
| { | |
| dateFrom: '2025-02-10T00:00:00Z', | |
| dateTo: '2025-02-17T00:00:00Z', | |
| averageTime: 66, | |
| unit: 'hours' | |
| }, | |
| { | |
| dateFrom: '2025-02-17T00:00:00Z', | |
| dateTo: '2025-02-24T00:00:00Z', | |
| averageTime: 44, | |
| unit: 'hours' | |
| }, | |
| { | |
| dateFrom: '2025-02-24T00:00:00Z', | |
| dateTo: '2025-03-03T00:00:00Z', | |
| averageTime: 97, | |
| unit: 'hours' | |
| } | |
| ] | |
| }; |
| <lfx-scroll-view id="review-time-by-pull-request-size" :observer="observer"> | ||
| <lfx-project-forks-review-time-by-pull-request-size /> | ||
| </lfx-scroll-view> | ||
| <lfx-scroll-view id="average-time-to-merge" :observer="observer"> | ||
| <lfx-project-average-time-to-merge /> | ||
| </lfx-scroll-view> | ||
| <lfx-scroll-view id="forks-wait-time-first-review" :observer="observer"> | ||
| <lfx-scroll-view id="wait-time-first-review" :observer="observer"> | ||
| <lfx-project-forks-wait-time-first-review /> | ||
| </lfx-scroll-view> |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify component name consistency with updated IDs.
While the scroll view IDs have been updated, the component names in the template still reference "forks" in their names (LfxProjectForksReviewTimeByPullRequestSize and LfxProjectForksWaitTimeFirstReview). Ensure these component names are consistent with your renaming strategy.
🏁 Script executed:
#!/bin/bash
# Check if the component names and imports are consistent with the new IDs
# Check the actual component file names
echo "Checking component file names:"
fd "review-time-by-pull-request-size.vue|wait-time-first-review.vue" --type f
# Check imports and component names in the code
echo "Checking component imports and usage:"
rg "LfxProjectForksReviewTimeByPullRequestSize|LfxProjectForksWaitTimeFirstReview" -A 1 -B 1Length of output: 2088
Update Component Names to Match Renamed IDs
The scroll view IDs have been updated (e.g., to "review-time-by-pull-request-size" and "wait-time-first-review"), but the corresponding component names still include the "Forks" prefix. To ensure consistency with the updated naming strategy, please update the components as follows:
-
In
frontend/app/components/modules/project/components/development/review-time-by-pull-request-size.vue:- Change the exported component name from
LfxProjectForksReviewTimeByPullRequestSizetoLfxProjectReviewTimeByPullRequestSize.
- Change the exported component name from
-
In
frontend/app/components/modules/project/components/development/wait-time-first-review.vue:- Change the exported component name from
LfxProjectForksWaitTimeFirstReviewtoLfxProjectWaitTimeFirstReview.
- Change the exported component name from
Also, update their usage in frontend/app/components/modules/project/views/development.vue accordingly. This will align the component names with the new scroll view IDs and maintain naming consistency.
| title="Merged" description="" icon="thumbs-up" :item-value="mergeLeadTime.data.pickup" | ||
| is-last /> |
There was a problem hiding this comment.
Verify usage of the pickup field for the "Merged" stage.
The line sets :item-value="mergeLeadTime.data.pickup" for the "Merged" title, which may be an oversight. You might intend to use another data field (e.g., mergeLeadTime.data.merged) for clarity and correctness.
- title="Merged" description="" icon="thumbs-up" :item-value="mergeLeadTime.data.pickup"
+ title="Merged" description="" icon="thumbs-up" :item-value="mergeLeadTime.data.merged"📝 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.
| title="Merged" description="" icon="thumbs-up" :item-value="mergeLeadTime.data.pickup" | |
| is-last /> | |
| title="Merged" description="" icon="thumbs-up" :item-value="mergeLeadTime.data.merged" | |
| is-last /> |
Signed-off-by: Efren Lim <elim@linuxfoundation.org>
Signed-off-by: Efren Lim <elim@linuxfoundation.org>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
frontend/server/api/project/[slug]/development/average-time-merge.get.ts (2)
21-26: Fix grammatical error in comment and consider future implementation.There's a small grammatical error in the time-period comment.
- * - time-period: string // This is isn't defined yet, but we'll add '90d', '1y', '5y' for now + * - time-period: string // This isn't defined yet, but we'll add '90d', '1y', '5y' for now
27-27: Implement query parameter handling and error handling.The endpoint currently returns mock data without using the query parameters documented above. For a production-ready implementation, you'll need to:
- Extract and use the query parameters
- Add error handling
- Eventually replace mock data with actual data retrieval logic
- export default defineEventHandler(async () => averageTimeMerge); + export default defineEventHandler(async (event) => { + try { + const query = getQuery(event); + const project = query.project as string; + const repository = query.repository as string; + const timePeriod = query['time-period'] as string; + + // For now, return mock data + // TODO: Replace with actual data retrieval using the parameters + return averageTimeMerge; + } catch (error) { + console.error('Error fetching average time to merge:', error); + throw createError({ + statusCode: 500, + statusMessage: 'Failed to fetch average time to merge data' + }); + } + });frontend/app/components/modules/project/components/development/average-time-to-merge.vue (5)
10-32: Consider adding fallback UI for error states.The component currently shows a spinner when status is 'pending' and the content when status is 'success', but doesn't handle the 'error' state with a user-friendly message. Consider adding an error state UI component to improve user experience.
<section class="mt-5"> <div v-if="status === 'success'" class="flex flex-row gap-4 items-center mb-6" > <div class="text-data-display-1">{{ formatNumberToDuration(summary.current) }}</div> <lfx-delta-display :summary="summary" icon="circle-arrow-up-right" icon-type="solid" is-duration /> </div> <div class="w-full h-[330px]"> <lfx-chart v-if="status !== 'pending'" :config="barChartConfig" /> <lfx-spinner v-else /> + <div v-if="status === 'error'" class="flex flex-col items-center justify-center h-full"> + <p class="text-neutral-500">Failed to load average time to merge data.</p> + <button class="mt-4 text-brand-500" @click="refresh">Retry</button> + </div> </div> </section>
65-74: Add data refresh capability for error recovery.The component does not have a way to refresh the data if the initial fetch fails. Consider adding a refresh function to allow users to retry when errors occur.
- const { data, status, error } = useFetch( + const { data, status, error, refresh } = useFetch( `/api/project/${route.params.slug}/development/average-time-merge`, { params: { repository: route.params.name || '', startDate, endDate, } } );
106-107: Extract magic numbers into named constants.The code uses the magic number
20multiple times in the y-axis max calculation. Consider extracting this into a named constant for better readability and maintainability.+ const Y_AXIS_INCREMENT = 20; + // Later in the code... yAxis: { axisLabel: { formatter: (value: number) => `${value === 0 ? '' : `${value}h`}` }, - max: (value: { min: number; max: number }) => Math.ceil(value.max / 20) * 20 + 20, + max: (value: { min: number; max: number }) => Math.ceil(value.max / Y_AXIS_INCREMENT) * Y_AXIS_INCREMENT + Y_AXIS_INCREMENT, }
113-122: Provide a more descriptive error message.The error message includes
error.value?.statusMessagewhich might be undefined. Consider providing a more descriptive default message.watch(error, (err) => { if (err) { showToast( - `Error fetching average time to merge: ${error.value?.statusMessage}`, + `Error fetching average time to merge: ${error.value?.statusMessage || 'Failed to retrieve data'}`, ToastTypesEnum.negative, undefined, - 10000 + 10000 // Consider extracting this to a constant like TOAST_DURATION_ERROR = 10000 ); } });
37-58: Consider organizing imports for better readability.The imports could be organized more systematically, for example grouping by:
- Vue/Nuxt core imports
- Component imports
- Utility/Helper imports
- Type imports
This would make the import section more maintainable as the component grows.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
frontend/app/components/modules/project/components/contributors/geographical-distribution.vue(3 hunks)frontend/app/components/modules/project/components/development/average-time-to-merge.vue(1 hunks)frontend/app/components/modules/project/components/development/contributions-outside-work-hours.vue(2 hunks)frontend/app/components/modules/project/components/development/merge-lead-time.vue(2 hunks)frontend/app/components/modules/project/views/development.vue(3 hunks)frontend/app/components/uikit/delta-display/delta-display.vue(3 hunks)frontend/server/api/project/[slug]/development/average-time-merge.get.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- frontend/app/components/modules/project/components/development/merge-lead-time.vue
- frontend/app/components/modules/project/components/development/contributions-outside-work-hours.vue
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/app/components/uikit/delta-display/delta-display.vue
- frontend/app/components/modules/project/components/contributors/geographical-distribution.vue
- frontend/app/components/modules/project/views/development.vue
🔇 Additional comments (5)
frontend/server/api/project/[slug]/development/average-time-merge.get.ts (2)
1-2: Import from mock data looks good.The import of mock data appears to be correctly set up for development purposes.
3-20: Well-structured response data documentation.The JSDoc comment effectively documents the expected data structure, making it clear what format the frontend should expect. This is good practice.
frontend/app/components/modules/project/components/development/average-time-to-merge.vue (3)
4-4: Title corrected from "Averate" to "Average".The typo in the component heading has been properly corrected.
6-6: Improved spacing with added margin.The added margin-bottom class enhances the spacing and improves the visual hierarchy.
96-111:Details
❓ Verification inconclusive
Verify chart configuration is responsive.
The chart configuration looks good but ensure the chart remains readable on smaller screens. Consider adding responsive configuration based on viewport size.
🏁 Script executed:
#!/bin/bash # Let's check if there are any responsive configurations for the chart component # Look for responsive handling in the chart component rg "responsive|mediaQuery|breakpoints|viewportWidth" --include "*.vue" --include "*.ts" -A 3 -B 3 # Check for specific mobile/responsive handling in the bar chart config fd "bar.chart" --exec cat {} \; | grep -E "responsive|mediaQuery|breakpoints|viewportWidth" # Look for any media queries used with charts fd "chart" --type f --exec cat {} \; | grep -A 5 -B 5 "@media"Length of output: 700
Below is a new set of shell scripts designed to re‐verify whether any responsive configuration settings (such as media queries, breakpoints, etc.) exist in the codebase:
Once these scripts are executed, please verify if any responsive handling related to the chart configuration exists. Based on our initial investigation (which produced no output), it appears that there’s no dedicated responsive configuration for the chart.
Responsive Chart Configuration Check:
The computed chart configuration infrontend/app/components/modules/project/components/development/average-time-to-merge.vueappears to be static. There is no evidence (via our search) of responsive settings—such as viewport-based adjustments or media queries—applied to the chart. Please verify that this chart remains clear and readable on smaller screens. If responsiveness is handled elsewhere, ensure that the mobile breakout is still effective; otherwise, consider incorporating responsive configurations (e.g., dynamic dimension adjustments or media queries) to improve usability on mobile devices.
🏁 Script executed:
#!/bin/bash # Search for responsive configuration keywords in .vue and .ts files using the correct arguments for ripgrep rg -g "*.vue" -g "*.ts" "responsive|mediaQuery|breakpoints|viewportWidth" -A3 -B3Length of output: 83
🏁 Script executed:
#!/bin/bash # Search all .vue and .ts files for media query usage that might impact the chart rendering rg -g "*.vue" -g "*.ts" "@media" -A5 -B5Length of output: 40
Verify Chart Configuration for Responsiveness
The computed chart configuration in the file appears static. Our automated search for responsive keywords (such as media queries, breakpoints, or viewport-specific settings) did not return any results, which indicates that no dedicated responsive settings are in place. Please manually verify that the chart remains readable on smaller screens. If the chart appears cramped or unclear on mobile devices, consider adding responsive configurations—such as dynamic dimension adjustments or media queries—to ensure optimal usability.
| v-if="status === 'success'" | ||
| class="flex flex-row gap-4 items-center mb-6" | ||
| > | ||
| <div class="text-data-display-1">{{ formatNumberToDuration(summary.current) }}</div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null/undefined check for summary data.
The direct access to summary.current could cause runtime errors if the data isn't available or has an unexpected structure.
- <div class="text-data-display-1">{{ formatNumberToDuration(summary.current) }}</div>
+ <div class="text-data-display-1">{{ summary?.current ? formatNumberToDuration(summary.current) : 'N/A' }}</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 class="text-data-display-1">{{ formatNumberToDuration(summary.current) }}</div> | |
| <div class="text-data-display-1">{{ summary?.current ? formatNumberToDuration(summary.current) : 'N/A' }}</div> |
| const chartData = computed<ChartData[]>( | ||
| // convert the data to chart data | ||
| () => convertToChartData(averageTimeMerge.value.data as RawChartData[], 'dateFrom', [ | ||
| 'averageTime' | ||
| ]) | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null/undefined check for chartData computation.
The computed property for chartData doesn't handle the case when averageTimeMerge.value.data is null or undefined.
const chartData = computed<ChartData[]>(
// convert the data to chart data
- () => convertToChartData(averageTimeMerge.value.data as RawChartData[], 'dateFrom', [
+ () => {
+ if (!averageTimeMerge.value?.data) return [];
+ return convertToChartData(averageTimeMerge.value.data as RawChartData[], 'dateFrom', [
'averageTime'
- ])
+ ]);
+ }
);📝 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.
| const chartData = computed<ChartData[]>( | |
| // convert the data to chart data | |
| () => convertToChartData(averageTimeMerge.value.data as RawChartData[], 'dateFrom', [ | |
| 'averageTime' | |
| ]) | |
| ); | |
| const chartData = computed<ChartData[]>( | |
| // convert the data to chart data | |
| () => { | |
| if (!averageTimeMerge.value?.data) return []; | |
| return convertToChartData(averageTimeMerge.value.data as RawChartData[], 'dateFrom', [ | |
| 'averageTime' | |
| ]); | |
| } | |
| ); |
| const averageTimeMerge = computed<AverageTimeMerge>(() => data.value as AverageTimeMerge); | ||
|
|
||
| const summary = computed<Summary>(() => averageTimeMerge.value.summary); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null/undefined checks for computed properties.
The computed properties don't handle the case when data.value is null or undefined, which could lead to runtime errors.
- const averageTimeMerge = computed<AverageTimeMerge>(() => data.value as AverageTimeMerge);
+ const averageTimeMerge = computed<AverageTimeMerge>(() => {
+ return data.value ? (data.value as AverageTimeMerge) : { summary: {}, data: [] };
+ });
- const summary = computed<Summary>(() => averageTimeMerge.value.summary);
+ const summary = computed<Summary>(() => averageTimeMerge.value?.summary || {});📝 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.
| const averageTimeMerge = computed<AverageTimeMerge>(() => data.value as AverageTimeMerge); | |
| const summary = computed<Summary>(() => averageTimeMerge.value.summary); | |
| const averageTimeMerge = computed<AverageTimeMerge>(() => { | |
| return data.value ? (data.value as AverageTimeMerge) : { summary: {}, data: [] }; | |
| }); | |
| const summary = computed<Summary>(() => averageTimeMerge.value?.summary || {}); |
Signed-off-by: Efren Lim <elim@linuxfoundation.org>
2 participants
Merge PR: #56 first!
In this PR
Ticket
INS-111
Summary by CodeRabbit
New Features
Bug Fixes
Style