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>
WalkthroughThis pull request introduces several code enhancements across the frontend and server. In the pre-commit hook, an extra command is added to automatically stage changes fixed by linting. The active-days component now fetches data via an API with corresponding computed properties and a new TypeScript interface, while other Vue components have updates for conditional rendering and type safety. Chart configuration files have been improved for heatmap display, including new helper functions and type extensions. Additionally, a new API endpoint and mock data file for active days have been added. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant Hook as Pre-commit Hook
participant Git as Git
Dev->>Hook: Initiate commit
Hook->>Hook: Run linting and TypeScript checks (pnpm lint:fix)
Hook->>Git: Execute `git add .` to stage fixed files
Git-->>Dev: Proceed with commit if checks pass
sequenceDiagram
participant Comp as ActiveDays.vue
participant API as Active-Days API Endpoint
participant Mock as activeDays.mock
Comp->>API: Request active days data (with query parameters)
API->>Mock: Retrieve mock data
Mock-->>API: Return active days data
API-->>Comp: Deliver data
alt Successful fetch
Comp->>Comp: Update computed properties and render chart
else Error occurs
Comp->>Toast: Trigger error toast notification
end
Possibly related PRs
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Pro Plan! Simply use the command ✨ 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: 2
🔭 Outside diff range comments (1)
frontend/app/components/modules/project/components/popularity/stars.vue (1)
109-109: 🛠️ Refactor suggestionError message doesn't match component purpose
The error message mentions "active contributors" but this component displays "Stars" data.
Update the error message to correctly reflect the component's purpose:
- `Error fetching active contributors: ${error.value?.statusMessage}`, + `Error fetching stars data: ${error.value?.statusMessage}`,
🧹 Nitpick comments (5)
frontend/.husky/pre-commit (1)
6-6: Consider using a more targeted git add command.While adding
git add .after linting fixes is a common practice to automatically stage changes made by the linter, this command stages ALL changes in the working directory, which might include unrelated files or changes that weren't intended to be committed yet.A more targeted approach would be:
- git add . + git add -uUsing
git add -uwill only stage modifications to already tracked files, reducing the risk of accidentally committing unintended changes.frontend/app/components/uikit/chart/configs/heat-map.chart.ts (1)
29-30: Update xAxis configuration for better positioning.The offset addition is good, but you should make a decision on
boundaryGaprather than leaving it commented.-// boundaryGap: true, +boundaryGap: false, // Disabled to allow cells to align properly with axesfrontend/app/components/modules/project/components/development/active-days.vue (3)
6-9: Improve sentence structure for clarity.
You could rewrite this sentence to be more grammatically complete, for example:-Active contributor is an individual performing tasks such as commits, -issues, or pull requests during the selected time period. +An active contributor is an individual performing tasks such as commits, +issues, or pull requests during the selected time period.
12-25: Avoid redundant condition check forstatus === 'success'.
Since lines 12-25 are already guarded byv-if="status === 'success'", the nested check on line 21 is redundant and can be removed to simplify the template.-<span v-if="status === 'success'" class="text-xl"> +<span class="text-xl">
104-107: Correct the error message to align with active-days data.
The toast references “social mentions” instead of “active-days.” Update it to maintain consistency and clarity:-`Error fetching social mentions: ${error.value?.statusMessage}` +`Error fetching active days: ${error.value?.statusMessage}`
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
frontend/.husky/pre-commit(1 hunks)frontend/app/components/modules/project/components/development/active-days.vue(1 hunks)frontend/app/components/modules/project/components/development/pull-requests.vue(3 hunks)frontend/app/components/modules/project/components/development/types/active-days.types.ts(1 hunks)frontend/app/components/modules/project/components/popularity/forks.vue(2 hunks)frontend/app/components/modules/project/components/popularity/github-mentions.vue(1 hunks)frontend/app/components/modules/project/components/popularity/press-mentions.vue(1 hunks)frontend/app/components/modules/project/components/popularity/social-mentions.vue(1 hunks)frontend/app/components/modules/project/components/popularity/stars.vue(2 hunks)frontend/app/components/uikit/chart/configs/heat-map.chart.ts(5 hunks)frontend/app/components/uikit/chart/helpers/chart-helpers.ts(2 hunks)frontend/app/components/uikit/chart/types/ChartTypes.ts(1 hunks)frontend/server/api/projects/development/active-days.get.ts(1 hunks)frontend/server/mocks/active-days.mock.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/server/mocks/active-days.mock.ts
🔇 Additional comments (20)
frontend/app/components/modules/project/components/popularity/press-mentions.vue (1)
64-64: API endpoint path correctly relocated.The press mentions API was moved from the 'contributors' category to the 'popularity' category, which is a more logical categorization for this data.
frontend/app/components/modules/project/components/popularity/social-mentions.vue (1)
64-64: API endpoint path correctly relocated.The social mentions API was moved from the 'contributors' category to the 'popularity' category, which is a more logical categorization for this data.
frontend/app/components/modules/project/components/development/types/active-days.types.ts (1)
1-11: Well-structured interface for active days data.The ActiveDays interface is clearly defined with appropriate types for all properties. Good use of the shared Summary type and clear organization of the data structure needed for the active days feature.
frontend/app/components/modules/project/components/popularity/github-mentions.vue (1)
64-65: Correctly relocated API endpoint from contributors to popularity categoryThis change aligns the API endpoint with its actual purpose, moving it from
/api/projects/contributors/github-mentionsto/api/projects/popularity/github-mentionswhile maintaining the same functionality.frontend/app/components/uikit/chart/types/ChartTypes.ts (1)
10-10: Added heatmap chart type to support new active days widgetThe addition of the 'heatmap' type to the ChartSeries interface expands the chart capabilities to support the new active days widget mentioned in the PR objectives.
frontend/app/components/modules/project/components/popularity/stars.vue (2)
12-12: Added null check to prevent SSR errorsGood addition of the null check for the summary object. This aligns with the PR objective to "add logic checks to other widgets to prevent errors during server-side rendering (SSR)".
70-70: Improved type safety by accounting for undefined summaryUpdating the type of the summary computed property to handle the possibility of undefined values prevents potential runtime errors. This change complements the template change on line 12 for better defensive programming.
frontend/app/components/uikit/chart/helpers/chart-helpers.ts (2)
6-7: Include SeriesTypes and CategoryData types in imports.Good addition of these type references for ensuring type safety and better developer experience.
31-43: Well-implemented utility function for category data transformation.The new
convertToCategoryDatafunction efficiently transforms chart data into the appropriate format needed for categorical charts like heatmaps. The function correctly handles possible missing values with the fallback to 0 and ensures proper integer conversion.This function is a good abstraction that will make chart data preparation more consistent across components.
frontend/app/components/modules/project/components/development/pull-requests.vue (2)
22-23: Prevent server-side rendering errors for average velocity display.The conditional check ensures the velocity is only displayed when data is successfully loaded, preventing potential null reference errors during SSR.
100-102: Improved null safety with optional chaining.Good use of optional chaining to handle potential undefined values. This adds robustness against runtime errors when data is not yet available.
frontend/app/components/modules/project/components/popularity/forks.vue (2)
12-12: Enhanced conditional rendering to prevent null reference errors.Good addition of the
&& summarycheck to ensure that the content is only rendered when summary data is available.
70-70: Improved type safety with optional chaining and union type.Good update to the computed property type to allow for undefined values and use of optional chaining to safely access properties.
frontend/app/components/uikit/chart/configs/heat-map.chart.ts (6)
5-9: Better organization of imported types.Good reorganization of imported types for better readability and maintainability.
21-26: Improved grid positioning for heat map.Adding explicit grid margins improves the chart layout and provides better visual space around the heat map.
54-61: Enhanced visualization with color gradient.Good addition of the color gradient configuration for the heat map. The progression from neutral to brand colors provides better visual distinction between different data points.
100-111: Improved styling for heat map items.Good enhancement of the heat map cell styling with rounded corners, borders, and hover effects. This significantly improves the visual appeal and interactivity of the chart.
114-117: Better function signature formatting.Improved code readability by breaking the function parameters into separate lines.
159-166: Improved chart axis configuration.Good restructuring of the axis configuration code for better readability and maintainability. Using the categoryData to populate the axis data ensures proper data alignment.
frontend/app/components/modules/project/components/development/active-days.vue (1)
29-31: Verify handling of error state for chart rendering.
Currently, the chart is shown ifstatus !== 'pending', which includes “error” status. This can lead to an empty or broken chart visualization. Consider explicitly handling the error state in the template to avoid confusion or unexpected UI behavior.
| import { activeDays } from '~~/server/mocks/active-days.mock'; | ||
|
|
||
| /** | ||
| * Frontend expects the data to be in the following format: | ||
| * { | ||
| * summary: { | ||
| * current: number; // current value | ||
| * previous: number; // previous value | ||
| * percentageChange: number; // percentage change (return as actual percentage ex: 2.3 percent) | ||
| * changeValue: number; // change value | ||
| * periodFrom: string; // period from | ||
| * periodTo: string; // period to | ||
| * }, | ||
| * avgContributions: number; | ||
| * data: { | ||
| * dateFrom: string; // ISO 8601 date string - start of the bucket. Based on the interval | ||
| * dateTo: string; // ISO 8601 date string - end of the bucket. Based on the interval | ||
| * contributions: number; // count of contributions | ||
| * }[], | ||
| * } | ||
| */ | ||
| /** | ||
| * Query params: | ||
| * - project: string | ||
| * - repository: string | ||
| * - time-period: string // This is isn't defined yet, but we'll add '90d', '1y', '5y' for now | ||
| */ | ||
| export default defineEventHandler(async () => activeDays); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implemented new API endpoint for active days with mock data
This implements the new API endpoint for the active days widget, which was the main objective of this PR. The implementation is well-documented with clear comments about the expected data format and query parameters.
However, there's no actual implementation of query parameter handling - the function just returns the mock data regardless of the query parameters.
Consider implementing query parameter handling to ensure the endpoint behaves correctly with different parameters:
-export default defineEventHandler(async () => activeDays);
+export default defineEventHandler(async (event) => {
+ // Extract query parameters
+ const query = getQuery(event)
+ const project = query.project as string
+ const repository = query.repository as string
+ const timePeriod = query['time-period'] as string || '90d'
+
+ // For now, just return mock data
+ // TODO: Replace with actual implementation when available
+ return activeDays;
+});You'll need to import getQuery from h3 at the top of the file.
Committable suggestion skipped: line range outside the PR's diff.
| <div v-if="status === 'success'" class="flex flex-col gap-5"> | ||
| <lfx-project-pull-request-legend-item title="Open" :delta="openSummary!" :color="chartSeries[0]!.color!" /> | ||
| <lfx-project-pull-request-legend-item title="Merged" :delta="mergedSummary!" :color="chartSeries[1]!.color!" /> | ||
| <lfx-project-pull-request-legend-item title="Closed" :delta="closedSummary!" :color="chartSeries[2]!.color!" /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure legend items only render with successful data, but remove non-null assertions.
The conditional rendering is good, but using non-null assertions (!) is risky. Even with the v-if guard, it would be safer to handle potential undefined values properly.
-<lfx-project-pull-request-legend-item title="Open" :delta="openSummary!" :color="chartSeries[0]!.color!" />
-<lfx-project-pull-request-legend-item title="Merged" :delta="mergedSummary!" :color="chartSeries[1]!.color!" />
-<lfx-project-pull-request-legend-item title="Closed" :delta="closedSummary!" :color="chartSeries[2]!.color!" />
+<lfx-project-pull-request-legend-item title="Open" :delta="openSummary || {}" :color="chartSeries[0]?.color || ''" />
+<lfx-project-pull-request-legend-item title="Merged" :delta="mergedSummary || {}" :color="chartSeries[1]?.color || ''" />
+<lfx-project-pull-request-legend-item title="Closed" :delta="closedSummary || {}" :color="chartSeries[2]?.color || ''" />📝 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-if="status === 'success'" class="flex flex-col gap-5"> | |
| <lfx-project-pull-request-legend-item title="Open" :delta="openSummary!" :color="chartSeries[0]!.color!" /> | |
| <lfx-project-pull-request-legend-item title="Merged" :delta="mergedSummary!" :color="chartSeries[1]!.color!" /> | |
| <lfx-project-pull-request-legend-item title="Closed" :delta="closedSummary!" :color="chartSeries[2]!.color!" /> | |
| <div v-if="status === 'success'" class="flex flex-col gap-5"> | |
| <lfx-project-pull-request-legend-item title="Open" :delta="openSummary || {}" :color="chartSeries[0]?.color || ''" /> | |
| <lfx-project-pull-request-legend-item title="Merged" :delta="mergedSummary || {}" :color="chartSeries[1]?.color || ''" /> | |
| <lfx-project-pull-request-legend-item title="Closed" :delta="closedSummary || {}" :color="chartSeries[2]?.color || ''" /> |
2 participants
In this PR
Ticket
INS-116
Summary by CodeRabbit
New Features
UI Enhancements
Chart Enhancements