Implement charts for health metrics #1633
Conversation
Summary by CodeRabbit
Summary by CodeRabbit
WalkthroughThis change introduces a comprehensive project health visualization section to the Project page. It adds new backend fields and properties for health requirements, extends the frontend data model, integrates ApexCharts for chart rendering, implements reusable chart components, updates the UI to display health metrics and trends, and includes new unit and end-to-end tests. Changes
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (5)
✨ 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: 3
♻️ Duplicate comments (1)
frontend/src/types/card.ts (1)
41-42: Synchronise prop name with callerIf you adopt the earlier renaming suggestion, remember to rename the prop here:
- healthMetricsData?: HealthMetricsProps[] + healthMetrics?: HealthMetricsProps[]
🧹 Nitpick comments (13)
frontend/src/app/projects/[projectKey]/page.tsx (2)
91-91: Keep TODOs actionable or create an issueThe inline TODO is easy to forget once the PR is merged. Prefer converting this into a GitHub issue (referencing the line) or tracking it in a ticket so it does not slip through the cracks.
93-99: Prop-name drift between caller and calleeYou pass
healthMetricsData={project.healthMetrics}, whereas the parent object useshealthMetricsand the type inProjectmirrors that. Maintaining two names for the same payload (healthMetricsvs.healthMetricsData) increases cognitive load and can lead to accidental typos.Consider normalising on a single prop/property name (
healthMetrics) throughout:- healthMetricsData={project.healthMetrics} + healthMetrics={project.healthMetrics}and update
DetailsCardPropsaccordingly.frontend/src/types/project.ts (1)
22-23: Make the collection read-onlyHealth metrics should be an immutable snapshot for the UI layer. Declare the property as a read-only tuple to prevent accidental in-place mutation by consumer components:
- healthMetrics?: HealthMetricsProps[] + readonly healthMetrics?: readonly HealthMetricsProps[]Helps TypeScript narrow immutability errors early.
backend/tests/apps/owasp/graphql/nodes/project_test.py (1)
47-51: Assertion could missListwrapper
field.type.of_typeworks whentypeisList[ProjectHealthMetricsNode]; if the schema later changes to a non-list field, this test will silently pass. A stricter assertion:assert getattr(field.type, "of_type", None) is ProjectHealthMetricsNode assert getattr(field.type, "_of_type", None) is None # guards against nested listsor use Strawberry’s
is_list_of_type.frontend/src/components/CardDetailsPage.tsx (2)
17-17: Consider lazy-loading heavy chart bundle
HealthMetricsChartslikely pulls in ApexCharts (~100 kB). Importing it eagerly increases the initial JS payload for every details page, even those that are not projects. A simpleReact.lazy/ dynamicimport()keeps non-project pages lighter.-import HealthMetricsCharts from 'components/HealthMetricsCharts' +const HealthMetricsCharts = React.lazy( + () => import('components/HealthMetricsCharts'), +)Remember to wrap the usage in
<Suspense>(or your app’s equivalent).
35-36: Prop should be optional in the component signature
healthMetricsDatais only relevant for projects. Make it optional (healthMetricsData?: …) inDetailsCardPropsto avoid forcing callers (e.g. chapter pages) to passundefined.backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_scores_test.py (2)
11-11: Magic number updated – assert the calculation insteadHard-coding
EXPECTED_SCORE = 34.0means every future tweak to the scoring formula will require manual test edits. Prefer deriving the expected score fromfields_weightsto keep the test self-maintaining.
76-81: Assertion covers onlyscorefield – include side-effectsYou assert
bulk_savecall and the finalscore. Consider also asserting thatmock_metric.updated_at(or similar timestamp) was modified if the command is expected to bump it, ensuring the full behaviour is protected.backend/apps/owasp/migrations/0041_merge_20250613_0336.py (1)
1-13: Silence Pylint false-positive for “too few public methods”Merge migrations are intentionally empty, but Pylint still complains.
A terse pragma keeps CI green without affecting Django:from django.db import migrations class Migration(migrations.Migration): + # pylint: disable=too-few-public-methodsfrontend/src/components/HealthMetricsCharts.tsx (2)
14-17: Reverse the dataset to chart chronological order
ProjectNode.health_metricsreturns records ordered descending (-nest_created_at).
When you map directly, Day 1 represents the most recent day and the chart runs right-to-left, which is counter-intuitive.-const openIssuesCountArray = data.map((item) => item.openIssuesCount) -const labels = data.map((item, index) => `Day ${index + 1}`) +const chronological = [...data].reverse() // oldest → newest +const openIssuesCountArray = chronological.map((i) => i.openIssuesCount) +const labels = chronological.map((_, idx) => `Day ${idx + 1}`)Optionally wrap those computations in
useMemoto avoid recalculation on each render.
86-99: Radial charts: one series per chart, not N seriesApexCharts’
radialBarexpectsseries: number[], one numeric value per radial bar.
Passing an array of many values causes a multi-series radial (concentric rings) which may not be what you want for “days since last commit / release”.If the intent is to display only the latest value, do:
-series={data.map((item) => item.lastCommitDays)} -labels={labels} +series={[chronological.at(-1)?.lastCommitDays ?? 0]} +labels={['Today']}Adjust similarly for
lastReleaseDays.backend/tests/apps/owasp/graphql/nodes/project_health_metrics_test.py (1)
36-44: Dependence on Strawberry internals is brittleThe tests reach into
__strawberry_definition__, an internal implementation detail that may change across Strawberry releases.
A safer approach is to validate the schema via Strawberry’s public API or GraphQL introspection:schema = strawberry.Schema(query=Query) introspection = schema._schema # or use graphql-core introspectionThis keeps the test future-proof.
frontend/src/components/GeneralCharts.tsx (1)
20-28: Explicitly set the charttypeto avoid silent behavioural changesApexCharts currently defaults to
line, but being explicit guards against future default changes and improves readability.- <Chart + <Chart + type="line" options={{ xaxis: { categories: labels, }, }} series={series} height={450} />
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (23)
backend/apps/owasp/graphql/nodes/project.py(2 hunks)backend/apps/owasp/graphql/nodes/project_health_metrics.py(1 hunks)backend/apps/owasp/management/commands/owasp_update_project_health_metrics_scores.py(1 hunks)backend/apps/owasp/migrations/0038_projecthealthmetrics_has_long_open_issues_and_more.py(1 hunks)backend/apps/owasp/migrations/0041_merge_20250613_0336.py(1 hunks)backend/apps/owasp/migrations/0042_remove_projecthealthmetrics_has_no_recent_commits_and_more.py(1 hunks)backend/apps/owasp/migrations/0043_remove_projecthealthmetrics_has_recent_commits_and_more.py(1 hunks)backend/apps/owasp/migrations/0044_merge_20250615_0346.py(1 hunks)backend/apps/owasp/migrations/0045_remove_projecthealthmetrics_has_long_open_issues_and_more.py(1 hunks)backend/apps/owasp/models/project_health_metrics.py(1 hunks)backend/tests/apps/owasp/graphql/nodes/project_health_metrics_test.py(1 hunks)backend/tests/apps/owasp/graphql/nodes/project_test.py(2 hunks)backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_scores_test.py(3 hunks)cspell/custom-dict.txt(1 hunks)frontend/package.json(2 hunks)frontend/src/app/projects/[projectKey]/page.tsx(1 hunks)frontend/src/components/CardDetailsPage.tsx(3 hunks)frontend/src/components/GeneralCharts.tsx(1 hunks)frontend/src/components/HealthMetricsCharts.tsx(1 hunks)frontend/src/server/queries/projectQueries.ts(1 hunks)frontend/src/types/card.ts(2 hunks)frontend/src/types/healthMetrics.ts(1 hunks)frontend/src/types/project.ts(2 hunks)
🧰 Additional context used
🪛 Pylint (3.3.7)
backend/apps/owasp/migrations/0043_remove_projecthealthmetrics_has_recent_commits_and_more.py
[refactor] 6-6: Too few public methods (0/2)
(R0903)
backend/apps/owasp/migrations/0041_merge_20250613_0336.py
[refactor] 6-6: Too few public methods (0/2)
(R0903)
backend/apps/owasp/migrations/0042_remove_projecthealthmetrics_has_no_recent_commits_and_more.py
[refactor] 6-6: Too few public methods (0/2)
(R0903)
backend/apps/owasp/migrations/0045_remove_projecthealthmetrics_has_long_open_issues_and_more.py
[refactor] 6-6: Too few public methods (0/2)
(R0903)
backend/apps/owasp/migrations/0044_merge_20250615_0346.py
[refactor] 6-6: Too few public methods (0/2)
(R0903)
backend/tests/apps/owasp/graphql/nodes/project_health_metrics_test.py
[error] 14-14: Class 'ProjectHealthMetricsNode' has no 'strawberry_definition' member
(E1101)
[error] 40-40: Class 'ProjectHealthMetricsNode' has no 'strawberry_definition' member
(E1101)
backend/apps/owasp/migrations/0038_projecthealthmetrics_has_long_open_issues_and_more.py
[refactor] 6-6: Too few public methods (0/2)
(R0903)
⏰ Context from checks skipped due to timeout of 90000ms (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 (14)
cspell/custom-dict.txt (1)
34-34: Add apexcharts to custom dictionary
This inclusion ensures the spell checker recognizes the new charting library name without false positives.frontend/package.json (1)
39-39: Include ApexCharts dependencies
Addingapexchartsand its React wrapper independenciescorrectly equips the frontend for rendering health metric charts.Also applies to: 58-58
backend/apps/owasp/migrations/0038_projecthealthmetrics_has_long_open_issues_and_more.py (1)
12-31: Skip review on auto-generated migration
This migration file is Django-generated schema change code; no manual edits needed.backend/apps/owasp/models/project_health_metrics.py (1)
26-26: Skip trivial formatting change
The added blank line afterforks_countis a harmless style adjustment.backend/apps/owasp/management/commands/owasp_update_project_health_metrics_scores.py (1)
63-68: Approve refactoredbulk_savecall formatting
The multi-line argument format enhances readability without altering behavior.frontend/src/types/project.ts (1)
2-3: Import location looks off
HealthMetricsPropslives intypes/healthMetrics, but other domain-level types (Issue, Release, …) live in sibling directories. IfhealthMetrics.tsended up in a different folder by mistake, move it next to the othertypesto keep project structure flat and predictable.frontend/src/types/card.ts (1)
6-7: Circular dependency risk
Cardtypes now pullHealthMetricsProps, which itself likely references project-level concepts. Watch out for circular type imports (types/healthMetrics→types/project→types/card). If you notice slow TS compile or weird “circular structure” warnings, extract shared primitives to a new module (e.g.types/metrics).backend/apps/owasp/migrations/0045_remove_projecthealthmetrics_has_long_open_issues_and_more.py (1)
11-28: Data loss auditDropping four boolean columns permanently deletes historical data. Confirm that:
- No downstream analytics or dashboards still rely on them.
- A backup/ETL has archived the values if they are useful for future reporting.
If both checks pass, migration looks correct.
backend/apps/owasp/migrations/0042_remove_projecthealthmetrics_has_no_recent_commits_and_more.py (1)
11-21: ```shell
#!/bin/bashLocate and display the contents of migration 0043 in the owasp app
migration_files=$(fd -e py '0043_.*.py' backend/apps/owasp/migrations)
if [ -z "$migration_files" ]; then
echo "No 0043 migration file found."
exit 1
fifor file in $migration_files; do
echo "== $file =="
sed -n '1,200p' "$file"
done</details> <details> <summary>backend/apps/owasp/migrations/0043_remove_projecthealthmetrics_has_recent_commits_and_more.py (1)</summary> `11-21`: ```shell #!/bin/bash # List any migration files with “0042” in their name echo "Migration files containing '0042':" ls backend/apps/owasp/migrations | grep '0042' || echo "No 0042 files found" # For each matching file, print its leading content to inspect the field operations for file in backend/apps/owasp/migrations/*0042*.py; do echo echo "===== $file =====" sed -n '1,200p' "$file" donebackend/apps/owasp/migrations/0044_merge_20250615_0346.py (1)
6-12: Merge migration is fine but indicates branching painsThe merge itself is harmless, but it’s another hint that the migration chain is oscillating. After consolidating 0042/0043, this merge will likely become unnecessary and can be deleted.
backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_scores_test.py (1)
39-54: Weights diverge: double-check with algorithmThe metric weights for
open_issues_count,unanswered_issues_count, andunassigned_issues_countare now higher (7). Confirm that the production scoring code uses the same values; otherwise the test will give false confidence.backend/apps/owasp/graphql/nodes/project.py (1)
39-44: ```shell
#!/bin/bash
set -erg "class BulkSaveModel" -A20 -n backend/apps/owasp/models
</details> <details> <summary>frontend/src/server/queries/projectQueries.ts (1)</summary> `15-25`: **Verify camel-casing conventions between backend & frontend** The query asks for `healthMetrics`, `forksCount`, etc., while the backend resolver defines `health_metrics` / `forks_count`. This only works when Strawberry’s `auto_camel_case` flag is enabled project-wide (it is by default, but can be disabled). Please double-check that: 1. `settings.STRAWBERRY_SETTINGS["auto_camel_case"]` (or equivalent) is not overridden; 2. No per-module override disables camel-casing. If camel-case is turned off, this query will fail at runtime. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
ahmedxgouda
force-pushed
the
dashboard/frontend-implementation
branch
from
9fcac7b to
c033d35
Compare
ahmedxgouda
force-pushed
the
dashboard/frontend-implementation
branch
from
c033d35 to
9623f53
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
cspell/custom-dict.txt(1 hunks)frontend/__tests__/unit/data/mockProjectDetailsData.ts(1 hunks)frontend/__tests__/unit/pages/ProjectDetails.test.tsx(1 hunks)frontend/package.json(2 hunks)frontend/src/app/projects/[projectKey]/page.tsx(1 hunks)frontend/src/components/CardDetailsPage.tsx(3 hunks)frontend/src/components/GeneralCharts.tsx(1 hunks)frontend/src/components/HealthMetricsCharts.tsx(1 hunks)frontend/src/server/queries/projectQueries.ts(1 hunks)frontend/src/types/card.ts(2 hunks)frontend/src/types/healthMetrics.ts(1 hunks)frontend/src/types/project.ts(2 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 (10)
- cspell/custom-dict.txt
- frontend/src/types/project.ts
- frontend/src/types/card.ts
- frontend/src/app/projects/[projectKey]/page.tsx
- frontend/package.json
- frontend/src/components/CardDetailsPage.tsx
- frontend/src/types/healthMetrics.ts
- frontend/src/server/queries/projectQueries.ts
- frontend/src/components/HealthMetricsCharts.tsx
- frontend/src/components/GeneralCharts.tsx
🧰 Additional context used
🪛 GitHub Actions: Run CI/CD
frontend/__tests__/unit/pages/ProjectDetails.test.tsx
[error] 25-25: CSpell: Unknown word 'apexchart' found during spelling check.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: CodeQL (python)
ahmedxgouda
force-pushed
the
dashboard/frontend-implementation
branch
from
a8afbfe to
88768dc
Compare
|
@ahmedxgouda I also just pushed some updates for Radial Chart styling. I realized that gradient fill works not as I expected it to, so switched that to use static gradient background image instead. Also, I realized that it would be super helpful to have a "legend" of what green and red mean on this scale. Could you try adding something like this to the radial chart ( Also, if we could pluralize the |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
frontend/src/components/GradientRadialChart.tsx (3)
20-38: Simplify checkpoint calculation logic.The checkpoint calculation can be streamlined as suggested in the previous review. The current approach with separate assignment and fallback is unnecessarily verbose.
Apply this refactor to make the logic more concise:
- let checkpoint = requirement - let showRed = false - const stops = [0, 100] - const greenColor = '#0ef94e' - const redColor = '#f94e0e' // Red color for the end of the gradient - const orangeColor = '#f9b90e' // Orange color for the middle of the gradient - const colorStops = [ - { - offset: 0, - color: greenColor, - opacity: 1, - }, - ] - if (days > requirement) { - checkpoint = days - showRed = true - } - // Ensure checkpoint is at least 1 to avoid division by zero - checkpoint = checkpoint || 1 + const stops = [0, 100] + const greenColor = '#0ef94e' + const redColor = '#f94e0e' + const orangeColor = '#f9b90e' + const colorStops = [ + { + offset: 0, + color: greenColor, + opacity: 1, + }, + ] + const showRed = days > requirement + const checkpoint = Math.max(days, requirement) || 1
23-25: Consider using theme-aware colors.The gradient colors are hard-coded and don't adapt to the theme. Consider using CSS custom properties or theme-based color values for better consistency with the overall design system.
Consider moving these to a theme-aware approach:
- const greenColor = '#0ef94e' - const redColor = '#f94e0e' // Red color for the end of the gradient - const orangeColor = '#f9b90e' // Orange color for the middle of the gradient + const greenColor = theme === 'dark' ? '#0ef94e' : '#22c55e' + const redColor = theme === 'dark' ? '#f94e0e' : '#ef4444' + const orangeColor = theme === 'dark' ? '#f9b90e' : '#f59e0b'
110-110: Consider making chart height configurable.The chart height is hard-coded at 400px. Consider making this configurable through props for better reusability.
Add height as an optional prop:
const GradientRadialChart: React.FC<{ title: string icon?: IconProp days: number requirement: number + height?: number -}> = ({ title, days, icon, requirement }) => { +}> = ({ title, days, icon, requirement, height = 400 }) => {Then use it in the Chart component:
- height={400} + height={height}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/components/GradientRadialChart.tsx(1 hunks)frontend/src/components/LineChart.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/components/LineChart.tsx
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Run Code Scan
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: CodeQL (python)
🔇 Additional comments (1)
frontend/src/components/GradientRadialChart.tsx (1)
1-122: Overall implementation looks solid with proper theme integration.The component successfully integrates ApexCharts with Next.js theming and provides a clean interface for visualizing health metrics. The dynamic import approach correctly handles SSR issues, and the pluralization utility is properly utilized.
| <Chart | ||
| key={theme} | ||
| options={{ | ||
| chart: { | ||
| animations: { | ||
| enabled: true, | ||
| speed: 1000, | ||
| }, | ||
| }, | ||
| plotOptions: { | ||
| radialBar: { | ||
| hollow: { | ||
| margin: 0, | ||
| }, | ||
| startAngle: -90, | ||
| endAngle: 90, | ||
| dataLabels: { | ||
| show: true, | ||
| name: { | ||
| show: false, | ||
| }, | ||
| value: { | ||
| formatter: () => `${days} ${pluralize(days, 'day', 'days')}`, | ||
| color: theme === 'dark' ? '#ececec' : '#1E1E2C', | ||
| fontSize: '20px', | ||
| show: true, | ||
| offsetY: 0, | ||
| }, | ||
| }, | ||
| track: { | ||
| background: theme === 'dark' ? '#1E1E2C' : '#ececec', | ||
| margin: 0, | ||
| }, | ||
| }, | ||
| }, | ||
| fill: { | ||
| type: 'gradient', | ||
| gradient: { | ||
| shade: theme, | ||
| type: 'horizontal', | ||
| shadeIntensity: 0.5, | ||
| stops: stops, | ||
| colorStops: colorStops, | ||
| }, | ||
| }, | ||
| }} | ||
| series={[requirement ? normalizedDays : 0]} | ||
| height={400} | ||
| type="radialBar" | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add accessibility attributes to the chart.
The chart lacks proper accessibility attributes for screen readers. Consider adding ARIA labels and roles to improve accessibility.
Add accessibility attributes:
<Chart
+ aria-label={`Health metric chart showing ${days} ${pluralize(days, 'day', 'days')} out of ${requirement} required`}
+ role="img"
key={theme}📝 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.
| <Chart | |
| key={theme} | |
| options={{ | |
| chart: { | |
| animations: { | |
| enabled: true, | |
| speed: 1000, | |
| }, | |
| }, | |
| plotOptions: { | |
| radialBar: { | |
| hollow: { | |
| margin: 0, | |
| }, | |
| startAngle: -90, | |
| endAngle: 90, | |
| dataLabels: { | |
| show: true, | |
| name: { | |
| show: false, | |
| }, | |
| value: { | |
| formatter: () => `${days} ${pluralize(days, 'day', 'days')}`, | |
| color: theme === 'dark' ? '#ececec' : '#1E1E2C', | |
| fontSize: '20px', | |
| show: true, | |
| offsetY: 0, | |
| }, | |
| }, | |
| track: { | |
| background: theme === 'dark' ? '#1E1E2C' : '#ececec', | |
| margin: 0, | |
| }, | |
| }, | |
| }, | |
| fill: { | |
| type: 'gradient', | |
| gradient: { | |
| shade: theme, | |
| type: 'horizontal', | |
| shadeIntensity: 0.5, | |
| stops: stops, | |
| colorStops: colorStops, | |
| }, | |
| }, | |
| }} | |
| series={[requirement ? normalizedDays : 0]} | |
| height={400} | |
| type="radialBar" | |
| /> | |
| <Chart | |
| aria-label={`Health metric chart showing ${days} ${pluralize(days, 'day', 'days')} out of ${requirement} required`} | |
| role="img" | |
| key={theme} | |
| options={{ | |
| chart: { | |
| animations: { | |
| enabled: true, | |
| speed: 1000, | |
| }, | |
| }, | |
| plotOptions: { | |
| radialBar: { | |
| hollow: { | |
| margin: 0, | |
| }, | |
| startAngle: -90, | |
| endAngle: 90, | |
| dataLabels: { | |
| show: true, | |
| name: { | |
| show: false, | |
| }, | |
| value: { | |
| formatter: () => `${days} ${pluralize(days, 'day', 'days')}`, | |
| color: theme === 'dark' ? '#ececec' : '#1E1E2C', | |
| fontSize: '20px', | |
| show: true, | |
| offsetY: 0, | |
| }, | |
| }, | |
| track: { | |
| background: theme === 'dark' ? '#1E1E2C' : '#ececec', | |
| margin: 0, | |
| }, | |
| }, | |
| }, | |
| fill: { | |
| type: 'gradient', | |
| gradient: { | |
| shade: theme, | |
| type: 'horizontal', | |
| shadeIntensity: 0.5, | |
| stops: stops, | |
| colorStops: colorStops, | |
| }, | |
| }, | |
| }} | |
| series={[requirement ? normalizedDays : 0]} | |
| height={400} | |
| type="radialBar" | |
| /> |
🤖 Prompt for AI Agents
In frontend/src/components/GradientRadialChart.tsx between lines 63 and 112, the
Chart component lacks accessibility attributes for screen readers. Add
appropriate ARIA attributes such as aria-label describing the chart's purpose
and role="img" to the Chart component's container or root element to improve
accessibility. Ensure these attributes provide meaningful context about the
chart content for assistive technologies.
There was a problem hiding this comment.
It is not available in the Chart component!
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
react-apexcharts accessibility attributes aria-label role support
💡 Result:
React-ApexCharts is a React wrapper for ApexCharts, a popular charting library. When it comes to accessibility, such as support for ARIA attributes, aria-label, and role, it's important to understand both what ApexCharts offers and how React-ApexCharts exposes or supports those features.
-
ARIA Attributes in ApexCharts:
ApexCharts has some built-in support for accessibility:- It generates SVG elements with roles and attributes like
role="img"by default on the chart container. - It allows setting
aria-labeland other ARIA attributes via its configuration options under thechartkey, particularlychart: { foreColor, sparkline, ... }. - You can provide
aria-labeloraria-describedbythrough options likechart: { toolbar: { autoSelected: 'zoom', show: false }, animations: { enabled: true }, ...}but explicit aria-labels might need to be manually added or customized.
- It generates SVG elements with roles and attributes like
-
React-ApexCharts Support:
- Since React-ApexCharts is a thin wrapper, it mostly passes props to ApexCharts.
- You can add ARIA attributes to the main chart container via props or by customizing the options passed to ApexCharts.
- The component itself does not have explicit props just for ARIA or accessibility attributes, so you'd typically handle this by customizing the chart options or wrapping the chart in a container div with appropriate accessibility attributes.
-
Customizing ARIA for React-ApexCharts:
- You can add an
aria-labelon the container div wrapping the chart. - You can pass accessibility-related options in the ApexCharts options, e.g.:
const options = { chart: { id: 'my-chart', type: 'line', // other chart options }, // ApexCharts does not have a dedicated accessibility config, but you can add custom attributes on render }; return ( <div role="img" aria-label="Sales line chart showing monthly revenue"> <ReactApexChart options={options} series={series} type="line" height={350} /> </div> );
- You can add an
-
Checking the latest documentation:
- It’s a good idea to check ApexCharts official docs and React-ApexCharts GitHub repo for any updates or contributions related to accessibility.
In conclusion:
- ApexCharts provides some basic ARIA support.
- React-ApexCharts does not have specialized ARIA props.
- You can and should add necessary ARIA attributes (like
aria-labelandrole) by wrapping the ReactApexChart component or using React props on the container. - For important and complex accessibility needs, consider augmenting the chart with descriptive text or accessible alternatives.
Would you like me to help with a specific React-ApexCharts code example implementing ARIA attributes?
@ahmedxgouda You're absolutely right! The react-apexcharts Chart component doesn't support those attributes directly.
The correct approach would be to wrap the Chart in a container div with accessibility attributes:
<SecondaryCard title={title} icon={icon}>
<div
role="img"
aria-label={`Health metric chart showing ${days} ${pluralize(days, 'day', 'days')} out of ${requirement} required`}
>
<Chart
key={theme}
options={{
// ... existing options
}}
series={[requirement ? normalizedDays : 0]}
height={400}
type="radialBar"
/>
</div>
<div className="mt-0 flex justify-around">
<span className="text-md text-gray-800 dark:text-white md:ml-4">Active</span>
<span className="text-md text-gray-800 dark:text-white md:mr-4">Stale</span>
</div>
</SecondaryCard>Thanks for the correction!
✏️ Learnings added
Learnt from: ahmedxgouda
PR: OWASP/Nest#1633
File: frontend/src/components/GradientRadialChart.tsx:67-116
Timestamp: 2025-06-21T12:21:32.328Z
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.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/src/components/GradientRadialChart.tsx (1)
67-113: Add accessibility wrapper for the Chart component.Based on previous discussions, the Chart component doesn't support ARIA attributes directly. You should wrap it in a container div with appropriate accessibility attributes to improve screen reader support.
Apply this accessibility improvement:
<SecondaryCard title={title} icon={icon}> + <div + role="img" + aria-label={`Health metric chart showing ${days} ${pluralize(days, 'day', 'days')} out of ${requirement} required`} + > <Chart key={theme} options={{ // ... existing options }} series={[requirement ? normalizedDays : 0]} height={400} type="radialBar" /> + </div>
🧹 Nitpick comments (1)
frontend/src/components/GradientRadialChart.tsx (1)
23-25: Consider using theme-based colors for better consistency.The hardcoded color values don't adapt to the theme. Consider using CSS custom properties or theme-based color variables to ensure consistency with the overall design system.
You could define theme-aware colors:
- const greenColor = '#0ef94e' - const redColor = '#f94e0e' // Red color for the end of the gradient - const orangeColor = '#f9b90e' // Orange color for the middle of the gradient + const greenColor = theme === 'dark' ? '#0ef94e' : '#22c55e' + const redColor = theme === 'dark' ? '#f94e0e' : '#ef4444' + const orangeColor = theme === 'dark' ? '#f9b90e' : '#f59e0b'
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
frontend/package.json(2 hunks)frontend/src/components/GradientRadialChart.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/package.json
🧰 Additional context used
🧠 Learnings (1)
frontend/src/components/GradientRadialChart.tsx (1)
Learnt from: ahmedxgouda
PR: OWASP/Nest#1633
File: frontend/src/components/GradientRadialChart.tsx:67-116
Timestamp: 2025-06-21T12:21:32.328Z
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.
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: CodeQL (python)
🔇 Additional comments (2)
frontend/src/components/GradientRadialChart.tsx (2)
34-38: Good implementation of inline assignment.The checkpoint assignment and division by zero protection are well implemented. The inline assignment
checkpoint = days || 1effectively handles the edge case while keeping the code concise.
110-110: Series calculation correctly handles domain constraints.The conditional logic
requirement ? normalizedDays : 0is appropriate given that requirements should never be 0 in this domain. This correctly treats invalid requirements as an edge case.
There was a problem hiding this comment.
@ahmedxgouda left a couple more comments!
I've also fixed the issue with extra space in firefox and will push the changes shortly.
|
|
||
| const HealthMetrics: React.FC<{ data: HealthMetricsProps[] }> = ({ data }) => { | ||
| const openIssuesCountArray = data.map((item) => item.openIssuesCount) | ||
| const labels = data.map((item, index) => `Day ${index + 1}`) |
There was a problem hiding this comment.
We need to update resolvers for all of these data points that we use on charts to also return the date. So this should be not just a number, but a date and number of Open issues at that date. So that it would show the date on the tooltip and on the x axis instead of just Day 1 etc.
Same for other data points.
| <h3 className="flex items-center gap-2 text-lg font-semibold text-gray-700 dark:text-gray-200"> | ||
| <FontAwesomeIcon icon={faHeart} /> | ||
| Health Metrics | ||
| </h3> | ||
| <div className="flex-grow border-t border-gray-300"></div> | ||
| </div> | ||
| <h4 className="mb-4 text-center text-lg font-semibold text-gray-700 dark:text-gray-200"> | ||
| Score: {data[0].score} |
There was a problem hiding this comment.
Can we remove this whole divider and add a score at the top of the page. It should be something like a circle of different background color (based on the score, again from green to red) and the score number.
We want it to be more prominent at the top of the page. It could also be clickable to scroll to the metrics charts lower on the page 🤔
There was a problem hiding this comment.
Another chart again or just a circle? @kasya
There was a problem hiding this comment.
@ahmedxgouda I meant that clicking it should scroll to these charts on the project page. Maybe be the first one Issues trend? 🤔 We can update that later on, not necessarily with this PR 👍🏼
frontend/public/img/gradient.png
Outdated
There was a problem hiding this comment.
Could you delete this file if we no longer use it?
| const normalizedRequirement = (requirement / checkpoint) * 100 | ||
|
|
||
| if (showRed) { | ||
| const orangeStop = normalizedRequirement / 2 |
There was a problem hiding this comment.
I had this case when with 5 days requirement it still showed all green even though technically it should have showed at least orange.
I can see we only set orange if showRed is truthy. Can we change this logic? Also, maybe we should start showing orange at 75% instead of 50%? I'm not sure, but would love to see how that looks with setups when project is almost at stale.
…gouda/Nest into pr/ahmedxgouda/1633
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
backend/apps/owasp/graphql/nodes/project_health_metrics.py (1)
31-33: Critical resolver issue remains unresolved from previous review.The existing resolver methods still return bound method objects instead of actual data values, which will cause serialization failures at runtime.
Also applies to: 41-43, 51-53, 56-58, 66-68
frontend/src/types/healthMetrics.ts (1)
1-14: Type mis-alignment with GraphQL schema persists — missing fields from backend.The interface is still missing
ageDaysandlastPullRequestDaysfields that exist in the GraphQL schema, which can lead to runtime undefined errors.Add the missing fields to maintain type safety:
export type HealthMetricsProps = { + ageDays: number createdAt: string forksCount: number lastCommitDays: number lastCommitDaysRequirement: number + lastPullRequestDays: number lastReleaseDays: number lastReleaseDaysRequirement: number openIssuesCount: number openPullRequestsCount: number score: number starsCount: number unassignedIssuesCount: number unansweredIssuesCount: number }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
backend/apps/owasp/graphql/nodes/project_health_metrics.py(3 hunks)backend/tests/apps/owasp/graphql/nodes/project_health_metrics_test.py(3 hunks)frontend/src/components/CardDetailsPage.tsx(4 hunks)frontend/src/components/GradientRadialChart.tsx(1 hunks)frontend/src/components/HealthMetrics.tsx(1 hunks)frontend/src/components/LineChart.tsx(1 hunks)frontend/src/server/queries/projectQueries.ts(1 hunks)frontend/src/types/healthMetrics.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- frontend/src/server/queries/projectQueries.ts
- frontend/src/components/CardDetailsPage.tsx
- frontend/src/components/HealthMetrics.tsx
- frontend/src/components/LineChart.tsx
- backend/tests/apps/owasp/graphql/nodes/project_health_metrics_test.py
- frontend/src/components/GradientRadialChart.tsx
🧰 Additional context used
🪛 Pylint (3.3.7)
backend/apps/owasp/graphql/nodes/project_health_metrics.py
[error] 38-38: Instance of 'ProjectHealthMetricsNode' has no 'nest_created_at' member
(E1101)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Run backend tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (2)
backend/apps/owasp/graphql/nodes/project_health_metrics.py (1)
35-38: ```shell
#!/bin/bash
set -eShow GraphQL node definition including created_at resolver
sed -n '1,200p' backend/apps/owasp/graphql/nodes/project_health_metrics.py | sed -n '1,60p'
</details> <details> <summary>frontend/src/types/healthMetrics.ts (1)</summary> `16-19`: **LGTM! Clean chart data type definition.** The `ApexChartLabelSeries` type is well-structured for chart data visualization. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| @strawberry.field | ||
| def last_release_days_requirement(self) -> int: | ||
| """Resolve last release age requirement in days.""" | ||
| return self.last_release_days_requirement |
There was a problem hiding this comment.
Critical issue: Incorrect resolver implementation.
This new field follows the same problematic pattern, returning the method object instead of the actual integer value.
Apply this fix:
- @strawberry.field
- def last_release_days_requirement(self) -> int:
- """Resolve last release age requirement in days."""
- return self.last_release_days_requirement
+ @strawberry.field(name="lastReleaseDaysRequirement")
+ def resolve_last_release_days_requirement(self) -> int:
+ """Last release age requirement in days."""
+ from typing import cast
+ return cast(ProjectHealthMetrics, self).last_release_days_requirement📝 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.
| @strawberry.field | |
| def last_release_days_requirement(self) -> int: | |
| """Resolve last release age requirement in days.""" | |
| return self.last_release_days_requirement | |
| @strawberry.field(name="lastReleaseDaysRequirement") | |
| def resolve_last_release_days_requirement(self) -> int: | |
| """Last release age requirement in days.""" | |
| from typing import cast | |
| return cast(ProjectHealthMetrics, self).last_release_days_requirement |
🤖 Prompt for AI Agents
In backend/apps/owasp/graphql/nodes/project_health_metrics.py around lines 60 to
63, the resolver method last_release_days_requirement incorrectly returns the
method itself instead of the integer value. Fix this by returning the actual
attribute or value representing the last release days requirement, not the
method object.
| @strawberry.field | ||
| def last_commit_days_requirement(self) -> int: | ||
| """Resolve last commit age requirement in days.""" | ||
| return self.last_commit_days_requirement |
There was a problem hiding this comment.
Critical issue: Incorrect resolver implementation.
This new field follows the same problematic pattern as the existing resolvers, returning the method object instead of the actual integer value.
Apply this fix:
- @strawberry.field
- def last_commit_days_requirement(self) -> int:
- """Resolve last commit age requirement in days."""
- return self.last_commit_days_requirement
+ @strawberry.field(name="lastCommitDaysRequirement")
+ def resolve_last_commit_days_requirement(self) -> int:
+ """Last commit age requirement in days."""
+ from typing import cast
+ return cast(ProjectHealthMetrics, self).last_commit_days_requirement📝 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.
| @strawberry.field | |
| def last_commit_days_requirement(self) -> int: | |
| """Resolve last commit age requirement in days.""" | |
| return self.last_commit_days_requirement | |
| @strawberry.field(name="lastCommitDaysRequirement") | |
| def resolve_last_commit_days_requirement(self) -> int: | |
| """Last commit age requirement in days.""" | |
| from typing import cast | |
| return cast(ProjectHealthMetrics, self).last_commit_days_requirement |
🤖 Prompt for AI Agents
In backend/apps/owasp/graphql/nodes/project_health_metrics.py around lines 45 to
48, the resolver method last_commit_days_requirement incorrectly returns the
method object itself instead of the integer value. To fix this, rename the
method to avoid name collision with the attribute or access the attribute via
self with a different name, ensuring the method returns the integer value stored
in the instance attribute rather than the method object.
|
| @@ -1,4 +1,5 @@ | |||
| export type HealthMetricsProps = { | |||
| createdAt: string | |||
There was a problem hiding this comment.
It's not exactly working as it should regarding the gradient. This is why I feel like static image worked better. Let me add some screenshots to maybe help me explain.
Here are 2 different projects charts for releases:
Based on the second one - the first one should already have yellow/orange color showing on the fill (I marked approx. spot on the full circle chart).
Same for these two - looking at releases at the some spot - the commits chart should have some yellow-ish shade already:
Is this possible with the stops passed in? If not, we'll have to go back to static image.
There was a problem hiding this comment.
Actually, I don't feel the static image is a good idea.
Let me explain the logic further. We determined two checkpoints, one for the gradient to start going to orange and the other to start going to red. The fill of the gradient and the checkpoints are determined based on mathematical calculations. For example, the orange checkpoint is when the days hits 3/4 of the requirement. The red checkpoint is when the days exceeded the requirement. Any other case, the bar still green.
The bars are scaled based on their requirements. It is not reasonable to compare between two bars with different requirements, because they have different scales. The first two that you provided, not just they have different scales, the second one's scale is twice as the first one's. For the ending checkpoint, we determined that it is either the days or the requirement (just the bigger). So, here we can say that why the second two do not have the same color at the same bar position. They actually don't have the same scale! the second has a scale of 6 -which is the days- and the first has a skill of 5 - which is the requirement.
So, we can say, to resolve the issue that you stated. We need like a formula to get a fixed end checkpoint which does not depend on conditions, although this is so accurate.
The logic can be improved within time, we are close to the ideal approach. I don't think it is a good idea to just stick with the static image. You know, it is static, and may fail to represent the value accurately.
Sorry for this long text, just wanted to explain my thoughts. I'd like to hear your opinion of that of course. Thanks! @kasya
There was a problem hiding this comment.
Oooh, I finally got where the confusion is coming from for both of us! We were looking at this chart differently.
As I see it - even the full fill circle should not have 6 days as a scale as you mentioned above - it should still just be the requirement (so, 5). But if the project is passed that (say, 10 days) it should just show red, meaning that the project is stale and passed the requirement.
So, to simplify this, the days since last release should not affect the scale of the chart - it should only change the percentage of the "filled" part of the chart.
And that makes more sense to me, because for example, if we look at these 2 charts above - the one for commits has 2 days and 5 requirement. It's filled to approx. where 2 would be on that scale. But then on the left you have the same requirement (5) but the scale is different. I find it confusing tbh. I would that expect them to be different only if the requirement is different, because that's what represents the chart - number of days from active to stale.
Does that makes sense? what do you think?
There was a problem hiding this comment.
@ahmedxgouda we are discussing this with @arkid15r! Please do not work on any updates until further notice :) It might be we keep it this way.
| import React from 'react' | ||
| import type { HealthMetricsProps } from 'types/healthMetrics' | ||
| import GradientRadialChart from 'components/GradientRadialChart' | ||
| import LineChart from 'components/LineChart' | ||
|
|
||
| const HealthMetrics: React.FC<{ data: HealthMetricsProps[] }> = ({ data }) => { | ||
| const openIssuesCountArray = data.map((item) => item.openIssuesCount) | ||
| const labels = data.map((item, index) => `Day ${index + 1}`) | ||
| const labels = data.map((item) => { |
There was a problem hiding this comment.
Thanks for adding dates, but they are showing backwards 😅 I think it should be Jun 20th, and then Jun 21st. Could you order them by date (preferably on the backend)?
| @@ -49,11 +50,27 @@ const DetailsCard = ({ | |||
| geolocationData = null, | |||
| repositories = [], | |||
| }: DetailsCardProps) => { | |||
| let scoreStyle = 'bg-green-200 text-green-800' | |||
There was a problem hiding this comment.
Can we do it as a circle instead on the right side of the page. It should specify that this is a health score and show a number.
Here's a very rough sketch of what I had in mind!
There was a problem hiding this comment.
Also, for line charts - is there a way to customize y axes? I don't think it makes sense to show .5 on these - there can not be half a pull request or half a star.
Resolves #1622
Screencast.from.2025-06-20.19-36-54.mp4