Implement the detailed metrics dashboard page #1748
Conversation
Summary by CodeRabbit
WalkthroughThis change introduces new fields and resolvers to the backend GraphQL schema for project health metrics, adds corresponding properties to the frontend types, and implements a detailed project health metrics dashboard page in the frontend. It also refactors components and queries to accommodate these schema updates and improve the presentation of health metric details. Changes
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
frontend/src/components/DashboardCard.tsx (1)
16-16: Handle undefined className gracefully.The current template literal concatenation will include "undefined" in the class string when
classNameis not provided.- className={`overflow-hidden transition-colors duration-300 hover:bg-blue-100 dark:hover:bg-blue-950 ${className}`} + className={`overflow-hidden transition-colors duration-300 hover:bg-blue-100 dark:hover:bg-blue-950 ${className || ''}`}frontend/src/components/GeneralCompliantComponent.tsx (2)
13-14: Review hover color contrast for accessibility.The red hover state uses the same color for both light and dark modes (
hover:bg-red-600 dark:hover:bg-red-600), which might not provide optimal contrast in dark mode.- const redClass = 'bg-red-100 dark:bg-red-700 hover:bg-red-600 dark:hover:bg-red-600' + const redClass = 'bg-red-100 dark:bg-red-700 hover:bg-red-200 dark:hover:bg-red-600'
17-25: Consider adding accessibility attributes.The component would benefit from accessibility attributes to convey the compliance status to screen readers.
<SecondaryCard - className={clsx('pointer-events-auto items-center justify-center text-center font-light', { + className={clsx('pointer-events-auto items-center justify-center text-center font-light', { [greenClass]: compliant, [redClass]: !compliant, })} + role="status" + aria-label={compliant ? 'Compliant' : 'Non-compliant'} >frontend/src/components/MetricsScoreCircle.tsx (1)
20-22: Consider accessibility for pulsing animation.The pulsing animation for low scores might be problematic for users with motion sensitivity. Consider adding a
prefers-reduced-motioncheck.{score < 30 && ( - <div className="animate-pulse absolute inset-0 rounded-full bg-red-400/20"></div> + <div className="motion-safe:animate-pulse absolute inset-0 rounded-full bg-red-400/20"></div> )}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
backend/apps/owasp/graphql/nodes/project.py(1 hunks)backend/apps/owasp/graphql/nodes/project_health_metrics.py(3 hunks)backend/apps/owasp/models/project_health_metrics.py(2 hunks)backend/tests/apps/owasp/graphql/nodes/project_health_metrics_test.py(2 hunks)backend/tests/apps/owasp/graphql/queries/project_health_metrics_test.py(1 hunks)frontend/src/app/projects/[projectKey]/page.tsx(1 hunks)frontend/src/app/projects/dashboard/metrics/[projectKey]/page.tsx(1 hunks)frontend/src/components/CardDetailsPage.tsx(2 hunks)frontend/src/components/DashboardCard.tsx(1 hunks)frontend/src/components/GeneralCompliantComponent.tsx(1 hunks)frontend/src/components/MetricsScoreCircle.tsx(1 hunks)frontend/src/server/queries/projectQueries.ts(1 hunks)frontend/src/server/queries/projectsHealthDashboardQueries.ts(1 hunks)frontend/src/types/healthMetrics.ts(1 hunks)frontend/src/types/project.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
Learnt from: ahmedxgouda
PR: OWASP/Nest#1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a length check before rendering HealthMetrics: `healthMetricsData.length > 0`. This ensures that when HealthMetrics is rendered, the data array has at least one element, making accessing data[0] safe within the HealthMetrics component.
Learnt from: ahmedxgouda
PR: OWASP/Nest#1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a safety check that ensures HealthMetrics component is only rendered when healthMetricsData exists and has at least one element: `healthMetricsData && healthMetricsData.length > 0`. This makes accessing data[0] safe within the HealthMetrics component.
frontend/src/app/projects/[projectKey]/page.tsx (2)
Learnt from: ahmedxgouda
PR: OWASP/Nest#1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a length check before rendering HealthMetrics: `healthMetricsData.length > 0`. This ensures that when HealthMetrics is rendered, the data array has at least one element, making accessing data[0] safe within the HealthMetrics component.
Learnt from: ahmedxgouda
PR: OWASP/Nest#1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a safety check that ensures HealthMetrics component is only rendered when healthMetricsData exists and has at least one element: `healthMetricsData && healthMetricsData.length > 0`. This makes accessing data[0] safe within the HealthMetrics component.
frontend/src/types/project.ts (2)
Learnt from: ahmedxgouda
PR: OWASP/Nest#1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a length check before rendering HealthMetrics: `healthMetricsData.length > 0`. This ensures that when HealthMetrics is rendered, the data array has at least one element, making accessing data[0] safe within the HealthMetrics component.
Learnt from: ahmedxgouda
PR: OWASP/Nest#1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a safety check that ensures HealthMetrics component is only rendered when healthMetricsData exists and has at least one element: `healthMetricsData && healthMetricsData.length > 0`. This makes accessing data[0] safe within the HealthMetrics component.
frontend/src/components/CardDetailsPage.tsx (2)
Learnt from: ahmedxgouda
PR: OWASP/Nest#1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a length check before rendering HealthMetrics: `healthMetricsData.length > 0`. This ensures that when HealthMetrics is rendered, the data array has at least one element, making accessing data[0] safe within the HealthMetrics component.
Learnt from: ahmedxgouda
PR: OWASP/Nest#1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a safety check that ensures HealthMetrics component is only rendered when healthMetricsData exists and has at least one element: `healthMetricsData && healthMetricsData.length > 0`. This makes accessing data[0] safe within the HealthMetrics component.
frontend/src/components/DashboardCard.tsx (3)
Learnt from: ahmedxgouda
PR: OWASP/Nest#1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a length check before rendering HealthMetrics: `healthMetricsData.length > 0`. This ensures that when HealthMetrics is rendered, the data array has at least one element, making accessing data[0] safe within the HealthMetrics component.
Learnt from: ahmedxgouda
PR: OWASP/Nest#1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a safety check that ensures HealthMetrics component is only rendered when healthMetricsData exists and has at least one element: `healthMetricsData && healthMetricsData.length > 0`. This makes accessing data[0] safe within the HealthMetrics component.
Learnt from: ahmedxgouda
PR: OWASP/Nest#1703
File: frontend/src/components/BarChart.tsx:33-46
Timestamp: 2025-07-03T03:08:03.290Z
Learning: In the OWASP Nest project's BarChart component (frontend/src/components/BarChart.tsx), the days and requirements arrays are guaranteed to always have the same length in their use cases, so input validation for array length matching is not needed.
frontend/src/components/MetricsScoreCircle.tsx (2)
Learnt from: ahmedxgouda
PR: OWASP/Nest#1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a length check before rendering HealthMetrics: `healthMetricsData.length > 0`. This ensures that when HealthMetrics is rendered, the data array has at least one element, making accessing data[0] safe within the HealthMetrics component.
Learnt from: ahmedxgouda
PR: OWASP/Nest#1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a safety check that ensures HealthMetrics component is only rendered when healthMetricsData exists and has at least one element: `healthMetricsData && healthMetricsData.length > 0`. This makes accessing data[0] safe within the HealthMetrics component.
frontend/src/types/healthMetrics.ts (4)
Learnt from: ahmedxgouda
PR: OWASP/Nest#1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a length check before rendering HealthMetrics: `healthMetricsData.length > 0`. This ensures that when HealthMetrics is rendered, the data array has at least one element, making accessing data[0] safe within the HealthMetrics component.
Learnt from: ahmedxgouda
PR: OWASP/Nest#1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a safety check that ensures HealthMetrics component is only rendered when healthMetricsData exists and has at least one element: `healthMetricsData && healthMetricsData.length > 0`. This makes accessing data[0] safe within the HealthMetrics component.
Learnt from: ahmedxgouda
PR: OWASP/Nest#1633
File: frontend/src/components/GradientRadialChart.tsx:113-113
Timestamp: 2025-06-21T12:22:01.889Z
Learning: In the OWASP Nest project, health metrics requirements (like lastCommitDaysRequirement, lastReleaseDaysRequirement) should never be 0. A requirement value of 0 is considered invalid and should result in displaying 0 on the radial chart.
Learnt from: ahmedxgouda
PR: OWASP/Nest#1703
File: frontend/src/components/BarChart.tsx:33-46
Timestamp: 2025-07-03T03:08:03.290Z
Learning: In the OWASP Nest project's BarChart component (frontend/src/components/BarChart.tsx), the days and requirements arrays are guaranteed to always have the same length in their use cases, so input validation for array length matching is not needed.
backend/apps/owasp/graphql/nodes/project_health_metrics.py (3)
Learnt from: Rajgupta36
PR: OWASP/Nest#1717
File: backend/apps/mentorship/graphql/queries/module.py:39-50
Timestamp: 2025-07-11T15:57:56.648Z
Learning: In the OWASP Nest mentorship GraphQL queries, Strawberry GraphQL automatically converts between Django Module instances and ModuleNode types, so methods can return Module instances directly without manual conversion even when typed as ModuleNode.
Learnt from: Rajgupta36
PR: OWASP/Nest#1717
File: backend/apps/mentorship/graphql/mutations/program.py:166-166
Timestamp: 2025-07-13T05:55:46.436Z
Learning: In the OWASP Nest mentorship GraphQL mutations, Strawberry GraphQL automatically converts between Django Program instances and ProgramNode types, so mutations can return Program instances directly without manual conversion even when typed as ProgramNode, similar to the Module/ModuleNode pattern.
Learnt from: ahmedxgouda
PR: OWASP/Nest#1633
File: frontend/src/components/GradientRadialChart.tsx:113-113
Timestamp: 2025-06-21T12:22:01.889Z
Learning: In the OWASP Nest project, health metrics requirements (like lastCommitDaysRequirement, lastReleaseDaysRequirement) should never be 0. A requirement value of 0 is considered invalid and should result in displaying 0 on the radial chart.
backend/apps/owasp/models/project_health_metrics.py (1)
Learnt from: ahmedxgouda
PR: OWASP/Nest#1633
File: frontend/src/components/GradientRadialChart.tsx:113-113
Timestamp: 2025-06-21T12:22:01.889Z
Learning: In the OWASP Nest project, health metrics requirements (like lastCommitDaysRequirement, lastReleaseDaysRequirement) should never be 0. A requirement value of 0 is considered invalid and should result in displaying 0 on the radial chart.
frontend/src/app/projects/dashboard/metrics/[projectKey]/page.tsx (5)
Learnt from: ahmedxgouda
PR: OWASP/Nest#1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a length check before rendering HealthMetrics: `healthMetricsData.length > 0`. This ensures that when HealthMetrics is rendered, the data array has at least one element, making accessing data[0] safe within the HealthMetrics component.
Learnt from: ahmedxgouda
PR: OWASP/Nest#1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a safety check that ensures HealthMetrics component is only rendered when healthMetricsData exists and has at least one element: `healthMetricsData && healthMetricsData.length > 0`. This makes accessing data[0] safe within the HealthMetrics component.
Learnt from: ahmedxgouda
PR: OWASP/Nest#1703
File: frontend/src/components/BarChart.tsx:33-46
Timestamp: 2025-07-03T03:08:03.290Z
Learning: In the OWASP Nest project's BarChart component (frontend/src/components/BarChart.tsx), the days and requirements arrays are guaranteed to always have the same length in their use cases, so input validation for array length matching is not needed.
Learnt from: Rajgupta36
PR: OWASP/Nest#1717
File: frontend/src/components/ModuleCard.tsx:53-55
Timestamp: 2025-07-13T07:31:06.511Z
Learning: In Next.js 13+ app router, useRouter from 'next/navigation' does not provide asPath or query properties. Use useParams to extract route parameters and usePathname to get the current pathname instead.
Learnt from: Rajgupta36
PR: OWASP/Nest#1717
File: frontend/src/app/mentorship/programs/page.tsx:59-61
Timestamp: 2025-07-13T11:29:25.245Z
Learning: In Next.js 13+ app router, components with the 'use client' directive run entirely on the client side and don't require window object existence checks or SSR hydration considerations. Direct access to window.location and other browser APIs is safe in client components.
frontend/src/server/queries/projectQueries.ts (2)
Learnt from: ahmedxgouda
PR: OWASP/Nest#1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a length check before rendering HealthMetrics: `healthMetricsData.length > 0`. This ensures that when HealthMetrics is rendered, the data array has at least one element, making accessing data[0] safe within the HealthMetrics component.
Learnt from: ahmedxgouda
PR: OWASP/Nest#1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a safety check that ensures HealthMetrics component is only rendered when healthMetricsData exists and has at least one element: `healthMetricsData && healthMetricsData.length > 0`. This makes accessing data[0] safe within the HealthMetrics component.
frontend/src/server/queries/projectsHealthDashboardQueries.ts (2)
Learnt from: ahmedxgouda
PR: OWASP/Nest#1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a length check before rendering HealthMetrics: `healthMetricsData.length > 0`. This ensures that when HealthMetrics is rendered, the data array has at least one element, making accessing data[0] safe within the HealthMetrics component.
Learnt from: ahmedxgouda
PR: OWASP/Nest#1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a safety check that ensures HealthMetrics component is only rendered when healthMetricsData exists and has at least one element: `healthMetricsData && healthMetricsData.length > 0`. This makes accessing data[0] safe within the HealthMetrics component.
🧬 Code Graph Analysis (4)
frontend/src/types/project.ts (1)
frontend/src/types/healthMetrics.ts (1)
HealthMetricsProps(26-50)
backend/apps/owasp/graphql/nodes/project_health_metrics.py (1)
backend/apps/owasp/models/project_health_metrics.py (2)
age_days_requirement(91-93)owasp_page_last_update_days_requirement(134-136)
backend/apps/owasp/models/project_health_metrics.py (1)
backend/apps/owasp/graphql/nodes/project_health_metrics.py (4)
age_days_requirement(41-43)age_days(36-38)owasp_page_last_update_days_requirement(86-88)owasp_page_last_update_days(81-83)
frontend/src/app/projects/dashboard/metrics/[projectKey]/page.tsx (3)
frontend/src/types/healthMetrics.ts (1)
HealthMetricsProps(26-50)frontend/src/server/queries/projectsHealthDashboardQueries.ts (1)
GET_PROJECT_HEALTH_METRICS_DETAILS(21-51)frontend/src/app/global-error.tsx (1)
handleAppError(66-86)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
- GitHub Check: Run frontend unit tests
🔇 Additional comments (23)
frontend/src/app/projects/[projectKey]/page.tsx (1)
95-95: AllhealthMetricsreferences have been updated and verified
- No remaining occurrences of the old
healthMetricsprop in TS/TSX files.healthMetricsListis used consistently in:
frontend/src/types/project.tsfrontend/src/server/queries/projectQueries.tsfrontend/src/app/projects/[projectKey]/page.tsxhealthMetricsDatais passed and consumed correctly inCardDetailsPage, which includes thehealthMetricsData.length > 0safety check, and in theHealthMetricscomponent.No further action required.
frontend/src/types/project.ts (1)
22-22: Type definition rename is consistent with backend changes.The property rename from
healthMetricstohealthMetricsListmaintains the same type structure and optional nature, ensuring backward compatibility while aligning with the backend schema updates.backend/tests/apps/owasp/graphql/queries/project_health_metrics_test.py (1)
87-88: Good test coverage for new schema fields.The addition of
total_issues_countandtotal_releases_countfields to the test data ensures proper coverage for the expanded ProjectHealthMetricsNode schema. The test values are reasonable and maintain existing test logic.backend/tests/apps/owasp/graphql/nodes/project_health_metrics_test.py (2)
37-38: Comprehensive test coverage for new schema fields.The addition of
total_issues_countandtotal_releases_countto the expected field names ensures proper schema validation for the new fields.
74-75: Proper type validation for new fields.The type validation for
total_issues_countandtotal_releases_countasintfollows the existing test pattern and ensures schema correctness.frontend/src/server/queries/projectQueries.ts (1)
15-15: GraphQL query field rename is consistent with backend changes.The field rename from
healthMetrics(limit: 30)tohealthMetricsList(limit: 30)maintains the same query structure and parameters while aligning with the backend schema updates.frontend/src/components/DashboardCard.tsx (2)
10-12: Good API improvements for flexibility.Making
statsoptional and addingclassNameprop enhances the component's reusability.
20-20: LGTM on conditional stats rendering.The conditional rendering prevents empty content when stats is undefined.
frontend/src/components/GeneralCompliantComponent.tsx (1)
1-28: Well-structured compliance indicator component.The component is well-implemented with proper TypeScript types and clean conditional styling.
frontend/src/components/CardDetailsPage.tsx (2)
22-22: Good import addition for the new component.The MetricsScoreCircle import is correctly added.
66-66: Excellent refactoring to use MetricsScoreCircle component.Replacing inline styling logic with a dedicated component improves maintainability and reusability. The data access is safe based on the existing length check (
healthMetricsData.length > 0) as confirmed by the retrieved learnings.backend/apps/owasp/models/project_health_metrics.py (2)
90-93: LGTM on age_days_requirement property.The implementation follows the established pattern of other requirement properties and provides proper access to the project requirements data.
133-136: LGTM on owasp_page_last_update_days_requirement property.The implementation is consistent with the existing codebase pattern and provides the necessary requirement data access.
frontend/src/components/MetricsScoreCircle.tsx (1)
1-26: Well-designed health score visualization component.The component provides excellent visual feedback with appropriate color coding, hover effects, and animations. The circular design and gradient overlay create a professional appearance.
backend/apps/owasp/graphql/nodes/project.py (2)
40-46: LGTM: Method rename improves API clarity.The rename from
health_metricstohealth_metrics_listbetter describes that this method returns a list of metrics, improving API self-documentation.
48-57: Verified existence ofget_latest_health_metrics
Theget_latest_health_metricsmethod is defined inbackend/apps/owasp/models/project_health_metrics.pyat line 155, so thehealth_metrics_latestresolver will work as expected.frontend/src/types/healthMetrics.ts (1)
27-47: LGTM: Type extensions correctly represent schema changes.The new fields properly extend the HealthMetricsProps type to support the detailed dashboard functionality, with appropriate type annotations and consistent naming conventions.
backend/apps/owasp/graphql/nodes/project_health_metrics.py (2)
24-25: LGTM: New fields properly expose model attributes.The addition of
total_issues_countandtotal_releases_countto the fields list correctly exposes these model attributes through the GraphQL schema.
40-43: LGTM: New resolver methods follow established patterns.Both
age_days_requirementandowasp_page_last_update_days_requirementmethods correctly implement the resolver pattern and align with the corresponding model properties shown in the relevant code snippets.Also applies to: 85-88
frontend/src/app/projects/dashboard/metrics/[projectKey]/page.tsx (3)
32-41: LGTM: Component setup follows Next.js and Apollo Client best practices.The component correctly uses
useParamsfor route parameter extraction anduseQueryfor GraphQL data fetching with proper TypeScript typing.
43-50: LGTM: Error handling and data processing implemented correctly.The useEffect properly handles GraphQL errors with the global error handler and safely processes the nested data structure before updating state.
51-146: LGTM: Well-structured UI with proper component composition.The rendering logic properly handles loading states, uses conditional rendering for safety, and creates a well-organized dashboard layout with appropriate responsive design and reusable components.
frontend/src/server/queries/projectsHealthDashboardQueries.ts (1)
21-51: LGTM: Query correctly implements the extended schema.The new
GET_PROJECT_HEALTH_METRICS_DETAILSquery properly uses thehealthMetricsLatestfield and requests all the necessary fields to support the detailed dashboard functionality.
| const MetricsScoreCircle: FC<{ score: number }> = ({ score }) => { | ||
| let scoreStyle = 'bg-green-400 text-green-900' | ||
| if (score < 50) { | ||
| scoreStyle = 'bg-red-400 text-red-900' | ||
| } else if (score < 75) { | ||
| scoreStyle = 'bg-yellow-400 text-yellow-900' | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract score thresholds as constants.
The magic numbers for score thresholds should be extracted as constants for better maintainability and consistency with the backend thresholds.
+const SCORE_THRESHOLD_HEALTHY = 75
+const SCORE_THRESHOLD_NEED_ATTENTION = 50
+
const MetricsScoreCircle: FC<{ score: number }> = ({ score }) => {
let scoreStyle = 'bg-green-400 text-green-900'
- if (score < 50) {
+ if (score < SCORE_THRESHOLD_NEED_ATTENTION) {
scoreStyle = 'bg-red-400 text-red-900'
- } else if (score < 75) {
+ } else if (score < SCORE_THRESHOLD_HEALTHY) {
scoreStyle = 'bg-yellow-400 text-yellow-900'
}🤖 Prompt for AI Agents
In frontend/src/components/MetricsScoreCircle.tsx around lines 3 to 9, the score
thresholds 50 and 75 are hardcoded magic numbers. Extract these threshold values
into clearly named constants at the top of the file to improve maintainability
and ensure consistency with backend threshold definitions. Replace the hardcoded
numbers in the conditional logic with these constants.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/src/components/BarChart.tsx (1)
84-91: Apply optional chaining for cleaner code.The conditional color logic is well-implemented and adds valuable flexibility for different metric visualizations. However, the condition can be simplified using optional chaining.
Apply this diff to use optional chaining:
- if (reverseColors && reverseColors[dataPointIndex]) { + if (reverseColors?.[dataPointIndex]) {This change eliminates the redundant null check and makes the code more concise, as suggested by the static analysis tool.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
backend/apps/owasp/graphql/nodes/project_health_metrics.py(4 hunks)backend/apps/owasp/models/project_health_metrics.py(3 hunks)backend/tests/apps/owasp/graphql/nodes/project_test.py(1 hunks)frontend/__tests__/unit/data/mockProjectDetailsData.ts(1 hunks)frontend/src/app/projects/dashboard/metrics/[projectKey]/page.tsx(1 hunks)frontend/src/components/BarChart.tsx(2 hunks)frontend/src/components/GeneralCompliantComponent.tsx(1 hunks)frontend/src/server/queries/projectsHealthDashboardQueries.ts(1 hunks)frontend/src/types/healthMetrics.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/tests/unit/data/mockProjectDetailsData.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- frontend/src/components/GeneralCompliantComponent.tsx
- backend/apps/owasp/models/project_health_metrics.py
- frontend/src/app/projects/dashboard/metrics/[projectKey]/page.tsx
- backend/apps/owasp/graphql/nodes/project_health_metrics.py
- frontend/src/types/healthMetrics.ts
- frontend/src/server/queries/projectsHealthDashboardQueries.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ahmedxgouda
PR: OWASP/Nest#1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a length check before rendering HealthMetrics: `healthMetricsData.length > 0`. This ensures that when HealthMetrics is rendered, the data array has at least one element, making accessing data[0] safe within the HealthMetrics component.
Learnt from: ahmedxgouda
PR: OWASP/Nest#1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a safety check that ensures HealthMetrics component is only rendered when healthMetricsData exists and has at least one element: `healthMetricsData && healthMetricsData.length > 0`. This makes accessing data[0] safe within the HealthMetrics component.
frontend/src/components/BarChart.tsx (3)
Learnt from: ahmedxgouda
PR: OWASP/Nest#1703
File: frontend/src/components/BarChart.tsx:33-46
Timestamp: 2025-07-03T03:08:03.290Z
Learning: In the OWASP Nest project's BarChart component (frontend/src/components/BarChart.tsx), the days and requirements arrays are guaranteed to always have the same length in their use cases, so input validation for array length matching is not needed.
Learnt from: ahmedxgouda
PR: OWASP/Nest#1633
File: frontend/src/components/GradientRadialChart.tsx:67-116
Timestamp: 2025-06-21T12:21:32.372Z
Learning: The react-apexcharts Chart component does not support ARIA attributes like aria-label and role as direct props. To add accessibility attributes to ApexCharts in React, wrap the Chart component in a container div with the appropriate ARIA attributes.
Learnt from: ahmedxgouda
PR: OWASP/Nest#1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a length check before rendering HealthMetrics: `healthMetricsData.length > 0`. This ensures that when HealthMetrics is rendered, the data array has at least one element, making accessing data[0] safe within the HealthMetrics component.
🧬 Code Graph Analysis (1)
backend/tests/apps/owasp/graphql/nodes/project_test.py (3)
backend/tests/apps/owasp/graphql/nodes/project_health_metrics_test.py (1)
_get_field_by_name(44-52)backend/tests/apps/github/graphql/nodes/repository_test.py (1)
_get_field_by_name(41-44)backend/apps/owasp/graphql/nodes/project_health_metrics.py (1)
ProjectHealthMetricsNode(32-93)
🪛 Biome (1.9.4)
frontend/src/components/BarChart.tsx
[error] 84-84: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (2)
frontend/src/components/BarChart.tsx (1)
20-21: LGTM! Well-designed optional prop addition.The new
reverseColorsprop is appropriately typed as optional and properly integrated into the component signature.backend/tests/apps/owasp/graphql/nodes/project_test.py (1)
52-55: LGTM!The test correctly verifies that the
health_metrics_listfield exists and returns a list ofProjectHealthMetricsNodeobjects. The assertion pattern is consistent with other list field tests in the file.
| def test_resolve_health_metrics_latest(self): | ||
| field = self._get_field_by_name("health_metrics_latest") | ||
| assert field is not None | ||
| assert field.type.of_type is ProjectHealthMetricsNode |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the expected return type for health_metrics_latest field.
The test uses field.type.of_type which is typically used for list types, but the field name health_metrics_latest suggests it should return a single ProjectHealthMetricsNode object, not a list.
If health_metrics_latest returns a single object, the assertion should be:
- assert field.type.of_type is ProjectHealthMetricsNode
+ assert field.type is ProjectHealthMetricsNodeIf it returns a list, then the current assertion is correct.
🏁 Script executed:
#!/bin/bash
# Description: Check the actual field definition in the ProjectNode to determine the correct return type
# Expected: Find the field definition and see if it's a list or single object
ast-grep --pattern 'health_metrics_latest'Length of output: 161
Incorrect assertion of health_metrics_latest return type
The health_metrics_latest field is defined to return a single ProjectHealthMetricsNode (or null), not a list. The test should assert the field’s type directly, not its element type.
File: backend/tests/apps/owasp/graphql/nodes/project_test.py (lines 47–50)
- assert field.type.of_type is ProjectHealthMetricsNode
+ assert field.type is ProjectHealthMetricsNode📝 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.
| def test_resolve_health_metrics_latest(self): | |
| field = self._get_field_by_name("health_metrics_latest") | |
| assert field is not None | |
| assert field.type.of_type is ProjectHealthMetricsNode | |
| def test_resolve_health_metrics_latest(self): | |
| field = self._get_field_by_name("health_metrics_latest") | |
| assert field is not None | |
| assert field.type is ProjectHealthMetricsNode |
🤖 Prompt for AI Agents
In backend/tests/apps/owasp/graphql/nodes/project_test.py around lines 47 to 50,
the test incorrectly asserts that the health_metrics_latest field returns a list
by checking field.type.of_type. Instead, update the assertion to check
field.type directly against ProjectHealthMetricsNode, reflecting that the field
returns a single node or null, not a list.
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
Some initial thoughts 👍🏼 ⬇️
| 'Days Since Last Commit', | ||
| 'Days Since Last Release', | ||
| 'Days Since Last Pull Request', | ||
| 'Days Since OWASP Page Last Update', |
There was a problem hiding this comment.
Can we add a tooltip to this to show the full text when it gets truncated?
There was a problem hiding this comment.
This text is a part of the apexcharts Chart component. So, I don't have full control on it. Maybe it can be done, I will take a look.
There was a problem hiding this comment.
@ahmedxgouda Gotcha! I'll check that too! Thank you!
There was a problem hiding this comment.
@kasya Didn't find a way, but a tooltip will show when the user hover on the bar itself.
frontend/src/app/projects/dashboard/metrics/[projectKey]/page.tsx
Outdated
Show resolved
Hide resolved
frontend/src/app/projects/dashboard/metrics/[projectKey]/page.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
frontend/__tests__/e2e/pages/ProjectHealthDashboardMetricsDetails.spec.ts(1 hunks)frontend/__tests__/unit/data/mockProjectsDashboardMetricsDetailsData.ts(1 hunks)frontend/__tests__/unit/pages/ProjectHealthDashboardMetricsDetails.test.tsx(1 hunks)frontend/src/app/projects/dashboard/metrics/[projectKey]/page.tsx(1 hunks)frontend/src/components/GeneralCompliantComponent.tsx(1 hunks)frontend/src/server/queries/projectsHealthDashboardQueries.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/tests/unit/data/mockProjectsDashboardMetricsDetailsData.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/src/app/projects/dashboard/metrics/[projectKey]/page.tsx
- frontend/src/components/GeneralCompliantComponent.tsx
- frontend/src/server/queries/projectsHealthDashboardQueries.ts
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: ahmedxgouda
PR: OWASP/Nest#1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a length check before rendering HealthMetrics: `healthMetricsData.length > 0`. This ensures that when HealthMetrics is rendered, the data array has at least one element, making accessing data[0] safe within the HealthMetrics component.
Learnt from: ahmedxgouda
PR: OWASP/Nest#1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a safety check that ensures HealthMetrics component is only rendered when healthMetricsData exists and has at least one element: `healthMetricsData && healthMetricsData.length > 0`. This makes accessing data[0] safe within the HealthMetrics component.
frontend/__tests__/e2e/pages/ProjectHealthDashboardMetricsDetails.spec.ts (3)
Learnt from: ahmedxgouda
PR: OWASP/Nest#1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a length check before rendering HealthMetrics: `healthMetricsData.length > 0`. This ensures that when HealthMetrics is rendered, the data array has at least one element, making accessing data[0] safe within the HealthMetrics component.
Learnt from: ahmedxgouda
PR: OWASP/Nest#1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a safety check that ensures HealthMetrics component is only rendered when healthMetricsData exists and has at least one element: `healthMetricsData && healthMetricsData.length > 0`. This makes accessing data[0] safe within the HealthMetrics component.
Learnt from: Rajgupta36
PR: OWASP/Nest#1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
frontend/__tests__/unit/pages/ProjectHealthDashboardMetricsDetails.test.tsx (4)
Learnt from: ahmedxgouda
PR: OWASP/Nest#1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a length check before rendering HealthMetrics: `healthMetricsData.length > 0`. This ensures that when HealthMetrics is rendered, the data array has at least one element, making accessing data[0] safe within the HealthMetrics component.
Learnt from: ahmedxgouda
PR: OWASP/Nest#1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a safety check that ensures HealthMetrics component is only rendered when healthMetricsData exists and has at least one element: `healthMetricsData && healthMetricsData.length > 0`. This makes accessing data[0] safe within the HealthMetrics component.
Learnt from: Rajgupta36
PR: OWASP/Nest#1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.255Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
Learnt from: ahmedxgouda
PR: OWASP/Nest#1703
File: frontend/src/components/BarChart.tsx:33-46
Timestamp: 2025-07-03T03:08:03.290Z
Learning: In the OWASP Nest project's BarChart component (frontend/src/components/BarChart.tsx), the days and requirements arrays are guaranteed to always have the same length in their use cases, so input validation for array length matching is not needed.
🧬 Code Graph Analysis (2)
frontend/__tests__/e2e/pages/ProjectHealthDashboardMetricsDetails.spec.ts (1)
frontend/__tests__/unit/data/mockProjectsDashboardMetricsDetailsData.ts (1)
mockProjectsDashboardMetricsDetailsData(1-34)
frontend/__tests__/unit/pages/ProjectHealthDashboardMetricsDetails.test.tsx (1)
frontend/__tests__/unit/data/mockProjectsDashboardMetricsDetailsData.ts (1)
mockProjectsDashboardMetricsDetailsData(1-34)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
- GitHub Check: Run backend tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (5)
frontend/__tests__/e2e/pages/ProjectHealthDashboardMetricsDetails.spec.ts (1)
1-22: LGTM! Well-structured E2E test setup.The test setup properly mocks GraphQL responses, sets required CSRF token, and navigates to the correct page. The mock data structure aligns with the expected API response format.
frontend/__tests__/unit/pages/ProjectHealthDashboardMetricsDetails.test.tsx (4)
6-27: Excellent mock setup for isolated unit testing.The mocking strategy properly isolates the component under test by mocking external dependencies like ApexCharts, Apollo Client, Next.js navigation, and FontAwesome icons. This ensures the test focuses on the component's behavior rather than its dependencies.
45-69: Well-implemented loading and error state tests.Both tests properly mock the Apollo Client's
useQueryreturn values and verify the expected UI behavior for loading and error states. The use ofwaitForensures proper async handling of DOM updates.
86-88: LGTM! Synchronous forEach usage is acceptable here.Unlike the E2E test, this forEach loop uses synchronous DOM queries within
waitFor, which is perfectly acceptable. Thescreen.getByText()calls are synchronous operations that don't require individual awaiting.
32-43: Proper test lifecycle management.Good practice using
beforeEachto set up consistent mock state andafterEachto clear mocks, preventing test interference.
frontend/__tests__/e2e/pages/ProjectHealthDashboardMetricsDetails.spec.ts
Show resolved
Hide resolved
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
@ahmedxgouda I’ve pushed some updates to the Compliance badges styling. Hope that's ok with you.
This looks good for now as an MVP!
We’ll likely revisit this page later once I figure out a clearer way to highlight any missing requirements. The goal is to make it obvious to project admins which requirements are still unmet and how they impact the score, so they know what needs their attention.
Right now, I feel like only our Age BarChart communicates that somewhat clearly. I’ll need to spend a bit more time thinking about the best design approach for this.
If you have any ideas in the meantime, feel free to share too!
|
3 participants
Resolves #1702
Screencast.from.2025-07-20.17-34-08.mp4