feat: Add Contribution Heatmap to Chapter and Project Pages #2674
Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds per-day contribution maps and per-type contribution stats JSON fields to Chapter and Project; implements an aggregation management command; exposes fields via GraphQL with camel-cased stats; integrates heatmap and stats UI into pages; updates types, queries, tests, Makefiles, and codegen mappings. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
backend/apps/owasp/management/commands/owasp_aggregate_contributions.py (2)
208-231: Consider memory implications for large datasets.Loading all chapters into memory at once (line 217) could be problematic if there are many active chapters. For more resilient processing, consider either:
- Processing in batches
- Using
.iterator()to stream results- Adding progress indicators for long-running operations
Example with error handling and progress:
if entity_type in ["chapter", "both"]: chapter_queryset = Chapter.objects.filter(is_active=True) if key: chapter_queryset = chapter_queryset.filter(key=key) if offset: chapter_queryset = chapter_queryset[offset:] total_chapters = chapter_queryset.count() self.stdout.write(f"Processing {total_chapters} chapters...") chapters_to_update = [] for idx, chapter in enumerate(chapter_queryset.iterator(), 1): try: contribution_data = self.aggregate_chapter_contributions( chapter, start_date, ) chapter.contribution_data = contribution_data chapters_to_update.append(chapter) if idx % 100 == 0: self.stdout.write(f" Progress: {idx}/{total_chapters}") except Exception as e: self.stdout.write( self.style.WARNING(f" Failed to aggregate chapter {chapter.key}: {e}") ) if chapters_to_update: Chapter.bulk_save(chapters_to_update, fields=("contribution_data",)) self.stdout.write( self.style.SUCCESS(f"✓ Updated {len(chapters_to_update)} chapters"), )
234-257: Apply the same memory and error handling improvements for projects.The same concerns about memory usage and error handling apply here as in the chapter processing section.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/apps/owasp/management/commands/owasp_aggregate_contributions.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/apps/owasp/management/commands/owasp_aggregate_contributions.py (2)
backend/apps/github/models/commit.py (1)
Commit(11-110)backend/apps/owasp/models/project.py (1)
pull_requests(233-239)
🪛 Ruff (0.14.5)
backend/apps/owasp/management/commands/owasp_aggregate_contributions.py
4-4: typing.Dict is deprecated, use dict instead
(UP035)
backend/apps/owasp/management/commands/owasp_aggregate_contributions.py
Outdated
Show resolved
Hide resolved
backend/apps/owasp/management/commands/owasp_aggregate_contributions.py
Outdated
Show resolved
Hide resolved
backend/apps/owasp/management/commands/owasp_aggregate_contributions.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/apps/owasp/api/internal/nodes/chapter.py(1 hunks)backend/apps/owasp/api/internal/nodes/project.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-11T15:57:56.648Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 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.
Applied to files:
backend/apps/owasp/api/internal/nodes/chapter.pybackend/apps/owasp/api/internal/nodes/project.py
📚 Learning: 2025-07-13T05:55:46.436Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 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.
Applied to files:
backend/apps/owasp/api/internal/nodes/chapter.pybackend/apps/owasp/api/internal/nodes/project.py
🔇 Additional comments (1)
backend/apps/owasp/api/internal/nodes/chapter.py (1)
21-21: Add node-level tests for ChapterNode to match ProjectNode test coverage.The code change is correct and
contribution_datais properly exposed in the GraphQL schema. However, verification shows that ChapterNode is missing test coverage that exists for ProjectNode:
- ProjectNode has:
backend/tests/apps/owasp/api/internal/nodes/project_test.py(verifying schema fields and custom resolvers)- ChapterNode has: Only query tests at
backend/tests/apps/owasp/api/internal/queries/chapter_test.py(no node tests)Create
backend/tests/apps/owasp/api/internal/nodes/chapter_test.pywith test cases similar to ProjectNode tests to verify:
- ChapterNode has valid Strawberry definition
- All fields in the decorator (including
contribution_data) are properly exposed- Custom field resolvers (
created_at,geo_location,key,suggested_location) work correctly
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
backend/apps/owasp/management/commands/owasp_aggregate_contributions.py (4)
49-67: Consider database-level aggregation for better performance.The current implementation fetches all date values into memory and aggregates in Python. For repositories with many contributions, this could be inefficient.
Consider using Django's aggregation framework to let the database do the grouping:
from django.db.models import Count from django.db.models.functions import TruncDate def _aggregate_contribution_dates( self, queryset, date_field: str, contribution_map: dict[str, int], ) -> None: """Aggregate contribution dates from a queryset into the contribution map.""" # Use database aggregation instead of fetching all values aggregated = ( queryset .annotate(date_key=TruncDate(date_field)) .values('date_key') .annotate(count=Count('id')) ) for item in aggregated: if item['date_key']: date_key = item['date_key'].isoformat() contribution_map[date_key] = contribution_map.get(date_key, 0) + item['count']This approach:
- Performs grouping at the database level (more efficient)
- Reduces memory usage by fetching only aggregated counts
- Scales better for large datasets
218-242: Add prefetch_related to reduce database queries.The current implementation accesses
chapter.owasp_repositoryfor each chapter in the loop (line 232-236), which can cause N+1 query issues.Apply this optimization:
# Process chapters if entity_type in ["chapter", "both"]: - chapter_queryset = Chapter.objects.filter(is_active=True) + chapter_queryset = Chapter.objects.filter(is_active=True).select_related('owasp_repository') if key: chapter_queryset = chapter_queryset.filter(key=key)This prefetches the repository relationship in a single query, avoiding N additional queries when iterating through chapters.
244-268: Add prefetch_related for project repositories.Similar to chapters, accessing
project.repositories.all()andproject.owasp_repositoryinside the loop causes N+1 queries.Apply this optimization:
# Process projects if entity_type in ["project", "both"]: - project_queryset = Project.objects.filter(is_active=True) + project_queryset = Project.objects.filter(is_active=True).select_related( + 'owasp_repository' + ).prefetch_related('repositories') if key: project_queryset = project_queryset.filter(key=key)This prefetches both the
owasp_repository(one-to-one) andrepositories(many-to-many) relationships efficiently.
31-36: Consider validating the days parameter.The
--daysparameter accepts any integer, including negative values or extremely large values that could cause issues.Add validation in the handle method:
def handle(self, *args, **options): """Execute the command.""" entity_type = options["entity_type"] days = options["days"] # Validate days parameter if days < 1: self.stdout.write( self.style.ERROR("Error: --days must be a positive integer") ) return if days > 3650: # 10 years self.stdout.write( self.style.WARNING( f"Warning: --days is set to {days}, which may cause performance issues" ) ) # ... rest of the method
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/apps/owasp/management/commands/owasp_aggregate_contributions.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/apps/owasp/management/commands/owasp_aggregate_contributions.py (1)
backend/apps/github/models/commit.py (1)
Commit(11-110)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/tests/apps/owasp/api/internal/nodes/project_test.py (1)
16-41: Consider adding a dedicated test method for contribution_data type.Following the pattern of other fields in this test file (e.g.,
test_resolve_issues_count,test_resolve_key), consider adding a dedicated test to verify thecontribution_datafield type. This would strengthen test coverage for this core PR feature and align with the PR objectives to "Add tests covering resolvers and data formatting."Example test to add after line 106:
def test_resolve_contribution_data(self): field = self._get_field_by_name("contribution_data") assert field is not None # Verify the type matches the expected JSON/dict type used in your GraphQL schema # For Strawberry GraphQL with JSONScalar, this might be: # assert field.type is strawberry.scalars.JSON # Or check for dict if using a dict type annotation
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/tests/apps/owasp/api/internal/nodes/project_test.py(1 hunks)
🔇 Additional comments (1)
backend/tests/apps/owasp/api/internal/nodes/project_test.py (1)
19-19: LGTM! Field correctly added to expected fields.The addition of
contribution_datato the expected field names correctly validates that the new JSON field is exposed in the ProjectNode schema.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
backend/tests/apps/owasp/management/commands/owasp_aggregate_contributions_test.py (4)
161-173: Use modern mock assertion methods.The pattern
assert mock_bulk_save.calledis deprecated. Usemock_bulk_save.assert_called()ormock_bulk_save.assert_called_once()for clearer intent and better error messages.Apply this diff:
- assert mock_bulk_save.called + mock_bulk_save.assert_called_once()
177-189: Use modern mock assertion methods.Same as above—prefer
mock_bulk_save.assert_called_once()over the deprecatedassert mock_bulk_save.calledpattern.Apply this diff:
- assert mock_bulk_save.called + mock_bulk_save.assert_called_once()
195-224: Use modern mock assertion methods.Both assertions should use
assert_called_once()instead of the deprecated.calledattribute check.Apply this diff:
- assert mock_chapter_bulk_save.called - assert mock_project_bulk_save.called + mock_chapter_bulk_save.assert_called_once() + mock_project_bulk_save.assert_called_once()
227-242: Strengthen filter argument verification.The test verifies the filter was called but doesn't check that the
keyparameter was actually used. Consider verifying the filter arguments to ensure the key filtering logic works correctly.You could strengthen the assertion like this:
# Verify filter was called with the specific key call_args = mock_filter.call_args # Verify 'key' in the filter kwargs or positional Q objectOr use
assert_called_with()if the filter signature is predictable:mock_filter.assert_called_with(key="www-chapter-test")backend/tests/apps/owasp/api/internal/nodes/chapter_test.py (1)
15-32: Consider adding type validation tests for remaining fields.The test validates that
created_at,name, andsummaryfields exist, but unlike other fields (lines 39-82), they lack dedicated type validation tests.For completeness, add type validation tests for the remaining fields:
def test_resolve_created_at(self): field = self._get_field_by_name("created_at") assert field is not None # Verify expected datetime type from datetime import datetime # Adjust based on how Strawberry exposes datetime fields def test_resolve_name(self): field = self._get_field_by_name("name") assert field is not None assert field.type is str def test_resolve_summary(self): field = self._get_field_by_name("summary") assert field is not None assert field.type is str
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/tests/apps/owasp/api/internal/nodes/chapter_test.py(1 hunks)backend/tests/apps/owasp/management/commands/owasp_aggregate_contributions_test.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
backend/tests/apps/owasp/management/commands/owasp_aggregate_contributions_test.py (2)
backend/apps/github/models/commit.py (1)
Commit(11-110)backend/apps/owasp/management/commands/owasp_aggregate_contributions.py (3)
_aggregate_contribution_dates(49-67)aggregate_chapter_contributions(69-132)aggregate_project_contributions(134-201)
backend/tests/apps/owasp/api/internal/nodes/chapter_test.py (1)
backend/apps/owasp/api/internal/queries/chapter.py (1)
chapter(14-19)
🪛 Ruff (0.14.5)
backend/tests/apps/owasp/management/commands/owasp_aggregate_contributions_test.py
44-44: datetime.datetime() called without a tzinfo argument
(DTZ001)
45-45: datetime.datetime() called without a tzinfo argument
(DTZ001)
46-46: datetime.datetime() called without a tzinfo argument
(DTZ001)
78-78: datetime.datetime.now() called without a tz argument
(DTZ005)
82-82: datetime.datetime() called without a tzinfo argument
(DTZ001)
85-85: datetime.datetime() called without a tzinfo argument
(DTZ001)
88-88: datetime.datetime() called without a tzinfo argument
(DTZ001)
91-91: datetime.datetime() called without a tzinfo argument
(DTZ001)
115-115: datetime.datetime.now() called without a tz argument
(DTZ005)
119-119: datetime.datetime() called without a tzinfo argument
(DTZ001)
120-120: datetime.datetime() called without a tzinfo argument
(DTZ001)
123-123: datetime.datetime() called without a tzinfo argument
(DTZ001)
126-126: datetime.datetime() called without a tzinfo argument
(DTZ001)
129-129: datetime.datetime() called without a tzinfo argument
(DTZ001)
143-143: datetime.datetime.now() called without a tz argument
(DTZ005)
153-153: datetime.datetime.now() called without a tz argument
(DTZ005)
282-282: datetime.datetime.now() called without a tz argument
(DTZ005)
🔇 Additional comments (4)
backend/tests/apps/owasp/management/commands/owasp_aggregate_contributions_test.py (4)
14-36: LGTM! Well-structured test fixtures.The fixtures provide clean, reusable mocks with appropriate attributes for testing the command.
38-138: LGTM! Comprehensive aggregation tests.The tests properly mock model querysets and verify per-day aggregation logic, including edge cases like None dates and multiple contributions on the same day.
140-157: LGTM! Good edge case coverage.These tests appropriately verify behavior when chapters or projects lack repositories.
265-285: Time-based test has reasonable tolerance.The 1-second tolerance for verifying
start_datecalculation should be sufficient for most test environments. If this becomes flaky in CI, consider mockingdatetime.now()for deterministic behavior.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
backend/apps/owasp/management/commands/owasp_aggregate_contributions.py (2)
48-66: Consider using.iterator()to reduce memory usage for large datasets.For repositories with many thousands of contributions,
values_list(..., flat=True)loads all dates into memory at once. This can be memory-intensive and slow.Apply this diff to stream results:
def _aggregate_contribution_dates( self, queryset, date_field: str, contribution_map: dict[str, int], ) -> None: """Aggregate contribution dates from a queryset into the contribution map. Args: queryset: Django queryset to aggregate date_field: Name of the date field to aggregate on contribution_map: Dictionary to update with counts """ - dates = queryset.values_list(date_field, flat=True) - for date_value in dates: + for date_value in queryset.values_list(date_field, flat=True).iterator(): if date_value: date_key = date_value.date().isoformat() contribution_map[date_key] = contribution_map.get(date_key, 0) + 1
202-269: Consider wrapping bulk operations intransaction.atomic().The bulk save operations for chapters (Line 238) and projects (Line 264) are not wrapped in transactions. If an error occurs during bulk_save, you may end up with partial updates. Wrapping each entity type's processing in
transaction.atomic()ensures all-or-nothing updates.Example for chapter processing:
from django.db import transaction # Inside handle method if entity_type in ["chapter", "both"]: with transaction.atomic(): chapter_queryset = Chapter.objects.filter(is_active=True).select_related( "owasp_repository" ) # ... rest of chapter processing if chapters: Chapter.bulk_save(chapters, fields=("contribution_data",))Apply the same pattern to project processing.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/src/types/__generated__/graphql.tsis excluded by!**/__generated__/**
📒 Files selected for processing (3)
backend/apps/owasp/management/commands/owasp_aggregate_contributions.py(1 hunks)backend/apps/owasp/migrations/0066_chapter_contribution_data_project_contribution_data.py(1 hunks)backend/tests/apps/owasp/management/commands/owasp_aggregate_contributions_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/tests/apps/owasp/management/commands/owasp_aggregate_contributions_test.py
- backend/apps/owasp/migrations/0066_chapter_contribution_data_project_contribution_data.py
🧰 Additional context used
🧬 Code graph analysis (1)
backend/apps/owasp/management/commands/owasp_aggregate_contributions.py (1)
backend/apps/github/models/commit.py (1)
Commit(11-110)
🔇 Additional comments (1)
backend/apps/owasp/management/commands/owasp_aggregate_contributions.py (1)
68-200: Aggregation logic is correct and well-structured.Both methods properly:
- Handle missing repositories with early returns
- Filter by start_date consistently across all contribution types
- Exclude draft releases
- Use the helper method to eliminate duplication
backend/apps/owasp/management/commands/owasp_aggregate_contributions.py
Outdated
Show resolved
Hide resolved
backend/apps/owasp/management/commands/owasp_aggregate_contributions.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
frontend/src/app/projects/[projectKey]/page.tsx (1)
93-98: Duplicate of earlier refactoring suggestion.This date range calculation is identical to the one in the chapter page. See the earlier comment on lines 64-69 of frontend/src/app/chapters/[chapterKey]/page.tsx for the recommended refactoring to extract this into a shared utility function.
backend/tests/apps/owasp/api/internal/nodes/chapter_test.py (2)
34-52: Verify field nullability to prevent test failures with Optional-wrapped types.The identity checks (
field.type is str,field.type is bool) will fail at runtime if Strawberry wraps any of these fields inOptionaldue tonull=Truein the model definition. The past review raised this concern forowasp_repository, but the same pattern persists here forkey,country,region, andis_active.Run this script to check if any of these fields are nullable in the Chapter model:
#!/bin/bash # Check nullability of fields tested with identity checks ast-grep --pattern $'class Chapter($$$): $$$ key = $$$ $$$' ast-grep --pattern $'class RepositoryBasedEntityModel($$$): $$$ country = $$$ $$$ region = $$$ $$$ is_active = $$$ $$$'If any field has
null=True, update the corresponding test to handle theOptionalwrapper usingtyping.get_origin()to extract the inner type, as shown in the past review comment forowasp_repository.
54-57: Complete the type assertion for contribution_data.The test verifies field existence but omits type verification, unlike all other field tests in this class. The comment indicates it should be a JSON scalar type, but this isn't asserted.
Apply this diff to add the missing assertion:
+from strawberry.scalars import JSON + def test_resolve_contribution_data(self): field = self._get_field_by_name("contribution_data") assert field is not None - # contribution_data is a JSON scalar type + assert field.type is JSON
🧹 Nitpick comments (3)
frontend/src/app/chapters/[chapterKey]/page.tsx (1)
64-69: Extract date range calculation into a shared utility function.This date range calculation logic is duplicated identically in the project page (frontend/src/app/projects/[projectKey]/page.tsx, lines 93-98). Extract it into a reusable utility function to follow DRY principles.
Create a utility function in
utils/dateFormatter.tsor a newutils/contributionHelpers.ts:export function getContributionDateRange(): { startDate: string; endDate: string } { const today = new Date() const oneYearAgo = new Date(today) oneYearAgo.setFullYear(today.getFullYear() - 1) return { startDate: oneYearAgo.toISOString().split('T')[0], endDate: today.toISOString().split('T')[0], } }Then use it in both files:
- const today = new Date() - const oneYearAgo = new Date(today) - oneYearAgo.setFullYear(today.getFullYear() - 1) - const startDate = oneYearAgo.toISOString().split('T')[0] - const endDate = today.toISOString().split('T')[0] + const { startDate, endDate } = getContributionDateRange()backend/tests/apps/owasp/management/commands/owasp_aggregate_contributions_test.py (2)
18-18: Consider adding docstrings to magic methods (optional).While not critical for a test helper class, adding brief docstrings to
__iter__,__getitem__, and__len__would improve code documentation.As per static analysis hints.
Also applies to: 21-21, 30-30
258-258: Strengthen the assertion to verify filter arguments.The test only checks that
filterwas called but doesn't verify it was called with the specific key. Consider usingassert_called_withor inspectingcall_argsto ensure the key parameter is properly passed to the filter.Example:
- mock_chapter_model.objects.filter.assert_called() + # Verify filter was called with the specific key + call_args = mock_chapter_model.objects.filter.call_args + assert call_args[1].get('key') == "www-chapter-test" or 'key' in str(call_args)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/tests/apps/owasp/api/internal/nodes/chapter_test.py(1 hunks)backend/tests/apps/owasp/management/commands/owasp_aggregate_contributions_test.py(1 hunks)frontend/src/app/chapters/[chapterKey]/page.tsx(2 hunks)frontend/src/app/projects/[projectKey]/page.tsx(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-06-20T16:12:59.256Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 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.
Applied to files:
frontend/src/app/chapters/[chapterKey]/page.tsxfrontend/src/app/projects/[projectKey]/page.tsx
📚 Learning: 2025-06-20T16:12:59.256Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 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.
Applied to files:
frontend/src/app/projects/[projectKey]/page.tsx
🧬 Code graph analysis (2)
backend/tests/apps/owasp/management/commands/owasp_aggregate_contributions_test.py (1)
backend/apps/owasp/api/internal/queries/chapter.py (1)
chapter(14-19)
backend/tests/apps/owasp/api/internal/nodes/chapter_test.py (1)
backend/apps/owasp/api/internal/queries/chapter.py (1)
chapter(14-19)
🪛 Ruff (0.14.5)
backend/tests/apps/owasp/management/commands/owasp_aggregate_contributions_test.py
18-18: Missing docstring in magic method
(D105)
21-21: Missing docstring in magic method
(D105)
30-30: Missing docstring in magic method
(D105)
🔇 Additional comments (13)
frontend/src/app/chapters/[chapterKey]/page.tsx (2)
12-12: LGTM!The ContributionHeatmap import is correctly added.
72-98: LGTM!The conditional rendering of ContributionHeatmap is correctly implemented:
- Fragment wrapper appropriately handles multiple return elements.
- The conditional check safely guards against null/undefined contributionData and empty objects.
- Component props are correctly passed with appropriate title and unit labels for the chapter context.
frontend/src/app/projects/[projectKey]/page.tsx (2)
20-22: LGTM!The ContributionHeatmap import is correctly added.
101-134: LGTM!The conditional rendering of ContributionHeatmap is correctly implemented:
- Fragment wrapper appropriately handles multiple return elements.
- The conditional check safely guards against null/undefined contributionData and empty objects.
- Component props are correctly passed with appropriate title and unit labels for the project context.
- UI styling is consistent with the chapter page implementation.
backend/tests/apps/owasp/management/commands/owasp_aggregate_contributions_test.py (9)
34-57: LGTM!Test fixtures are well-structured and provide appropriate mock data for chapters and projects with their associated repositories.
58-83: LGTM!Test thoroughly covers the helper method's functionality including same-day aggregation and None value handling.
84-119: LGTM!Test effectively validates that contributions from multiple sources (commits, issues, PRs, releases) are correctly aggregated by day.
121-159: LGTM!Test properly validates project contribution aggregation including handling of multiple contributions on the same day.
160-178: LGTM!Edge case tests appropriately verify that entities without repositories return empty contribution maps.
195-210: LGTM!Test appropriately validates command execution for projects only. (See separate comment regarding
bulk_savemethod verification.)
211-243: LGTM!Test effectively validates that both chapters and projects are processed when entity_type is "both". (See separate comment regarding
bulk_savemethod verification.)
276-296: LGTM!Test properly validates that the custom days parameter correctly affects the start_date calculation, with appropriate tolerance for test execution time.
183-183:bulk_saveis a valid custom method—no changes needed.The codebase implements
bulk_saveas a custom method across multiple models (Chapter, Project, Sponsor, ProjectHealthMetrics, Post, Event, Committee). The test usage is consistent with the actual implementation.
backend/tests/apps/owasp/management/commands/owasp_aggregate_contributions_test.py
Show resolved
Hide resolved
mrkeshav-05
force-pushed
the
feature/contribution-heatmap-chapter-project
branch
from
989006d to
8dfdd75
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
backend/apps/owasp/management/commands/owasp_aggregate_contributions.py (1)
217-267: Consider addingselect_related/prefetch_relatedto avoid N+1 queries inhandle.
handle()currently buildschapters = list(Chapter.objects.filter(is_active=True)...)andprojects = list(Project.objects.filter(is_active=True)...)withoutselect_related("owasp_repository")orprefetch_related("repositories"). Sinceaggregate_chapter_contributionsandaggregate_project_contributionstouch these relations for each entity, this will trigger extra queries per chapter/project.For a scheduled aggregation over many entities, consider:
- For chapters:
.select_related("owasp_repository").- For projects:
.select_related("owasp_repository").prefetch_related("repositories").This keeps behavior identical while improving performance during bulk runs.
backend/tests/apps/owasp/management/commands/owasp_aggregate_contributions_test.py (1)
245-295: Complete thehandlescenario assertions (especially offset).
test_handle_with_offsetstill ends with a comment but does not assert the behavior it describes, so regressions in offset handling would go unnoticed. Similarly,test_handle_with_specific_keyonly checks that.objects.filterwas called, not that the key filtering path was exercised.Consider tightening these tests, for example:
@@ def test_handle_with_offset(...): - with mock.patch.object( - command, - "aggregate_chapter_contributions", - return_value={"2024-11-16": 1}, - ): - command.handle(entity_type="chapter", offset=2, days=365) - - # Verify that offset was applied - only 1 chapter should be processed (3 total - 2 offset) + with mock.patch.object( + command, + "aggregate_chapter_contributions", + return_value={"2024-11-16": 1}, + ) as mock_aggregate: + command.handle(entity_type="chapter", offset=2, days=365) + + # 3 total chapters, offset=2 -> only 1 should be processed + assert mock_aggregate.call_count == 1 + mock_chapter_model.bulk_save.assert_called_once() @@ def test_handle_with_specific_key(...): - # Verify filter was called with the specific key - mock_chapter_model.objects.filter.assert_called() + # Verify initial active filter and that we attempted to narrow by key + mock_chapter_model.objects.filter.assert_called_with(is_active=True) + # Optionally, track calls on the queryset.filter(...) as well if you wantThis keeps the tests aligned with their comments and makes the control flow around
offsetandkeymuch harder to accidentally break.
🧹 Nitpick comments (2)
frontend/src/app/projects/[projectKey]/page.tsx (1)
93-134: Heatmap integration looks solid; consider simplifying the wrapper styling.The 1‑year date range calculation and conditional
contributionDataguard are correct and align with the backend aggregation. The only minor concern is themin-h-screen+ explicit background wrapper around the heatmap, which may introduce a very tall section and override the page background differently from the DetailsCard section. Consider droppingmin-h-screen(or reusing the layout pattern from existing heatmap usages) if you want more consistent page flow and styling.frontend/src/app/chapters/[chapterKey]/page.tsx (1)
64-98: Consistent chapter heatmap wiring; same layout consideration as projects page.The contribution date range and conditional rendering are correct and mirror the project page, which is good for consistency. As with the project view, the
min-h-screen+ background wrapper around the heatmap can create an extra-tall, differently styled block; consider droppingmin-h-screenor aligning the container styling with nearby content for a smoother layout.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
frontend/src/types/__generated__/chapterQueries.generated.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/graphql.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/projectQueries.generated.tsis excluded by!**/__generated__/**
📒 Files selected for processing (15)
backend/apps/owasp/api/internal/nodes/chapter.py(1 hunks)backend/apps/owasp/api/internal/nodes/project.py(1 hunks)backend/apps/owasp/management/commands/owasp_aggregate_contributions.py(1 hunks)backend/apps/owasp/migrations/0066_chapter_contribution_data_project_contribution_data.py(1 hunks)backend/apps/owasp/models/chapter.py(1 hunks)backend/apps/owasp/models/project.py(1 hunks)backend/tests/apps/owasp/api/internal/nodes/chapter_test.py(1 hunks)backend/tests/apps/owasp/api/internal/nodes/project_test.py(1 hunks)backend/tests/apps/owasp/management/commands/owasp_aggregate_contributions_test.py(1 hunks)frontend/src/app/chapters/[chapterKey]/page.tsx(2 hunks)frontend/src/app/projects/[projectKey]/page.tsx(2 hunks)frontend/src/server/queries/chapterQueries.ts(1 hunks)frontend/src/server/queries/projectQueries.ts(1 hunks)frontend/src/types/chapter.ts(1 hunks)frontend/src/types/project.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- frontend/src/types/project.ts
- frontend/src/server/queries/projectQueries.ts
- frontend/src/server/queries/chapterQueries.ts
- backend/apps/owasp/api/internal/nodes/chapter.py
- backend/apps/owasp/models/chapter.py
- frontend/src/types/chapter.ts
- backend/tests/apps/owasp/api/internal/nodes/chapter_test.py
- backend/apps/owasp/migrations/0066_chapter_contribution_data_project_contribution_data.py
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-06-20T16:12:59.256Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 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.
Applied to files:
frontend/src/app/projects/[projectKey]/page.tsxfrontend/src/app/chapters/[chapterKey]/page.tsx
📚 Learning: 2025-06-20T16:12:59.256Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 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.
Applied to files:
frontend/src/app/projects/[projectKey]/page.tsx
📚 Learning: 2025-07-11T15:57:56.648Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 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.
Applied to files:
backend/apps/owasp/api/internal/nodes/project.py
📚 Learning: 2025-07-13T05:55:46.436Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 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.
Applied to files:
backend/apps/owasp/api/internal/nodes/project.py
🧬 Code graph analysis (2)
backend/tests/apps/owasp/management/commands/owasp_aggregate_contributions_test.py (1)
backend/apps/owasp/api/internal/queries/chapter.py (1)
chapter(14-19)
backend/apps/owasp/management/commands/owasp_aggregate_contributions.py (3)
backend/apps/github/models/commit.py (1)
Commit(11-110)backend/apps/owasp/api/internal/queries/chapter.py (1)
chapter(14-19)backend/tests/apps/owasp/management/commands/owasp_aggregate_contributions_test.py (1)
filter(26-28)
🪛 Ruff (0.14.5)
backend/tests/apps/owasp/management/commands/owasp_aggregate_contributions_test.py
18-18: Missing docstring in magic method
(D105)
21-21: Missing docstring in magic method
(D105)
30-30: Missing docstring in magic method
(D105)
🔇 Additional comments (5)
backend/apps/owasp/management/commands/owasp_aggregate_contributions.py (1)
48-200: Aggregation logic matches requirements and is well-factored.The shared
_aggregate_contribution_dateshelper plus the chapter/project aggregation methods correctly count commits, issues, PRs (bycreated_at) and non-draft releases (bypublished_at) into aYYYY-MM-DD -> countmap, and they safely return empty maps when no repositories are available. Usingrepository_id__infor projects andbulk_save(..., fields=("contribution_data",))inhandle()is a good fit for the volume this command may process.backend/tests/apps/owasp/api/internal/nodes/project_test.py (1)
16-41: ProjectNode test correctly covers the newcontribution_datafield.Including
"contribution_data"inexpected_field_nameskeeps the meta-configuration test in sync with the GraphQL node definition and will catch regressions if the field is dropped in the future.backend/apps/owasp/api/internal/nodes/project.py (1)
23-38: GraphQL exposure ofcontribution_datais minimal and correct.Adding
"contribution_data"to thefieldslist cleanly exposes the JSONField via Strawberry-Django without extra resolver code, and aligns with the updated tests and frontend usage.backend/apps/owasp/models/project.py (1)
100-105:contribution_datafield definition is appropriate for aggregated counts.Defining
contribution_dataas aJSONFieldwithdefault=dictand the documentedYYYY-MM-DD -> countmapping fits the aggregation command’s output and avoids shared mutable defaults. This should work well with the GraphQL exposure and frontend heatmap.backend/tests/apps/owasp/management/commands/owasp_aggregate_contributions_test.py (1)
34-178: Good coverage of aggregation helpers and edge cases.The tests for
_aggregate_contribution_dates, chapter/project aggregation (including multiple events per day), and the “no repository” paths give solid confidence in the command’s core logic without touching the database, and theMockQuerySetabstraction keeps the handle() tests focused on control flow rather than ORM details.
mrkeshav-05
force-pushed
the
feature/contribution-heatmap-chapter-project
branch
from
f3b713c to
da0bb63
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
backend/tests/apps/owasp/management/commands/owasp_aggregate_contributions_test.py (1)
260-275: Complete the test by adding assertions.The test has a comment stating "Verify that offset was applied" but lacks any assertions to actually verify this behavior. This issue was previously flagged but remains unresolved. The test is incomplete and will pass regardless of whether the offset logic works correctly.
Apply this diff to add the necessary assertions:
with mock.patch.object( command, "aggregate_chapter_contributions", return_value={"2024-11-16": 1}, - ): + ) as mock_aggregate: command.handle(entity_type="chapter", offset=2, days=365) # Verify that offset was applied - only 1 chapter should be processed (3 total - 2 offset) + assert mock_aggregate.call_count == 1, \ + "Expected aggregate to be called once for 1 remaining chapter after offset" + mock_chapter_model.bulk_save.assert_called_once()Based on past review comments
🧹 Nitpick comments (5)
frontend/src/components/ContributionHeatmap.tsx (2)
13-13: Remove unusedtitleprop.The
titleprop is defined in the interface but is never used in the component. This appears to be leftover from a refactor where title rendering was removed (around lines 114-182 in the old version).Apply this diff:
interface ContributionHeatmapProps { contributionData: Record<string, number> startDate: string endDate: string - title?: string unit?: string stats?: {
15-20: Remove unusedstatsprop.The
statsprop is defined but never referenced in the component body. If this was intended for future use or display, it should either be implemented or removed to avoid confusion.Apply this diff to remove the unused prop:
interface ContributionHeatmapProps { contributionData: Record<string, number> startDate: string endDate: string unit?: string - stats?: { - commits?: number - pullRequests?: number - issues?: number - total?: number - } variant?: 'default' | 'compact' }frontend/src/app/chapters/[chapterKey]/page.tsx (1)
158-169: Incorrect icon choice for "Total" metric.The
faCodeMergeicon semantically represents merge/pull request operations, not total contributions. Consider using a more appropriate icon likefaChartLine,faChartBar, orfaCalculatorto represent an aggregate total.Apply this diff:
+import { faChartLine } from '@fortawesome/free-solid-svg-icons' ... <div className="flex items-center gap-2"> <FontAwesomeIcon - icon={faCodeMerge} + icon={faChartLine} className="h-4 w-4 text-gray-600 dark:text-gray-400" />backend/tests/apps/owasp/management/commands/owasp_aggregate_contributions_test.py (2)
26-28: Consider documenting the filter method's mock behavior.The
filtermethod returnsselfwithout applying any actual filtering logic. While this is acceptable for simple test mocking, it could lead to misleading test results if tests inadvertently rely on filtering behavior. Consider adding a docstring or comment to make this limitation explicit.Apply this diff to clarify the behavior:
def filter(self, **kwargs): - # Return self to support filter chaining + # Mock filter: returns self without applying actual filtering logic. + # Tests should not depend on filter criteria being evaluated. return self
244-259: Strengthen assertion to verify filter arguments.The test verifies that
filterwas called but doesn't confirm it was called with the correct key argument. Consider usingassert_called_withto verify the exact filter criteria.Apply this diff to strengthen the assertion:
- # Verify filter was called with the specific key - mock_chapter_model.objects.filter.assert_called() + # Verify filter was called with the specific key + calls = mock_chapter_model.objects.filter.call_args_list + assert any(call.kwargs.get('key') == 'www-chapter-test' for call in calls), \ + "Expected filter to be called with key='www-chapter-test'"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
frontend/src/types/__generated__/chapterQueries.generated.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/graphql.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/projectQueries.generated.tsis excluded by!**/__generated__/**
📒 Files selected for processing (17)
backend/apps/owasp/api/internal/nodes/chapter.py(1 hunks)backend/apps/owasp/api/internal/nodes/project.py(1 hunks)backend/apps/owasp/management/commands/owasp_aggregate_contributions.py(1 hunks)backend/apps/owasp/migrations/0066_chapter_contribution_data_project_contribution_data.py(1 hunks)backend/apps/owasp/models/chapter.py(1 hunks)backend/apps/owasp/models/project.py(1 hunks)backend/tests/apps/owasp/api/internal/nodes/chapter_test.py(1 hunks)backend/tests/apps/owasp/api/internal/nodes/project_test.py(1 hunks)backend/tests/apps/owasp/management/commands/owasp_aggregate_contributions_test.py(1 hunks)frontend/src/app/board/[year]/candidates/page.tsx(2 hunks)frontend/src/app/chapters/[chapterKey]/page.tsx(3 hunks)frontend/src/app/projects/[projectKey]/page.tsx(3 hunks)frontend/src/components/ContributionHeatmap.tsx(3 hunks)frontend/src/server/queries/chapterQueries.ts(1 hunks)frontend/src/server/queries/projectQueries.ts(1 hunks)frontend/src/types/chapter.ts(1 hunks)frontend/src/types/project.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- backend/apps/owasp/api/internal/nodes/project.py
- backend/apps/owasp/management/commands/owasp_aggregate_contributions.py
- backend/apps/owasp/migrations/0066_chapter_contribution_data_project_contribution_data.py
- backend/apps/owasp/models/chapter.py
- frontend/src/app/board/[year]/candidates/page.tsx
- frontend/src/server/queries/chapterQueries.ts
- frontend/src/app/projects/[projectKey]/page.tsx
- backend/tests/apps/owasp/api/internal/nodes/project_test.py
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-06-21T12:21:32.372Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 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.
Applied to files:
frontend/src/components/ContributionHeatmap.tsx
📚 Learning: 2025-07-13T11:29:25.245Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 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.
Applied to files:
frontend/src/app/chapters/[chapterKey]/page.tsx
📚 Learning: 2025-07-13T07:31:06.511Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 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.
Applied to files:
frontend/src/app/chapters/[chapterKey]/page.tsx
📚 Learning: 2025-07-11T15:57:56.648Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 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.
Applied to files:
backend/apps/owasp/api/internal/nodes/chapter.py
📚 Learning: 2025-07-13T05:55:46.436Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 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.
Applied to files:
backend/apps/owasp/api/internal/nodes/chapter.py
🧬 Code graph analysis (2)
backend/tests/apps/owasp/api/internal/nodes/chapter_test.py (1)
backend/apps/owasp/api/internal/queries/chapter.py (1)
chapter(14-19)
backend/tests/apps/owasp/management/commands/owasp_aggregate_contributions_test.py (2)
backend/apps/owasp/management/commands/owasp_aggregate_contributions.py (5)
Command(16-269)_aggregate_contribution_dates(48-66)aggregate_chapter_contributions(68-131)aggregate_project_contributions(133-200)handle(202-269)backend/apps/owasp/api/internal/queries/chapter.py (1)
chapter(14-19)
🪛 Ruff (0.14.5)
backend/tests/apps/owasp/management/commands/owasp_aggregate_contributions_test.py
18-18: Missing docstring in magic method
(D105)
21-21: Missing docstring in magic method
(D105)
30-30: Missing docstring in magic method
(D105)
🔇 Additional comments (17)
frontend/src/components/ContributionHeatmap.tsx (1)
263-276: LGTM: Variant-based styling is well-structured.The dynamic CSS generation with variant-aware class names and responsive breakpoints is implemented correctly. The use of template literals and conditional logic for compact vs. default variants is clean and maintainable.
frontend/src/types/chapter.ts (1)
21-21: LGTM: Type definition aligns with backend schema.The
contributionDatafield correctly mirrors the backend JSONField and GraphQL exposure.frontend/src/types/project.ts (1)
49-49: LGTM: Consistent type definition.The
contributionDatafield is correctly defined and matches the pattern used in Chapter type.frontend/src/server/queries/projectQueries.ts (1)
92-92: LGTM: GraphQL query correctly extended.The
contributionDatafield is properly added to the query and will fetch the aggregated contribution data from the backend.backend/apps/owasp/api/internal/nodes/chapter.py (1)
21-21: LGTM: GraphQL field exposure is correct.The
contribution_datafield is properly added to the ChapterNode fields list, exposing the backend model field through GraphQL.frontend/src/app/chapters/[chapterKey]/page.tsx (1)
71-76: LGTM: Date range calculation is correct.The 1-year lookback calculation properly constructs the date range for the heatmap.
backend/apps/owasp/models/project.py (1)
100-105: LGTM: Model field properly configured.The
contribution_dataJSONField is correctly defined with appropriate defaults, verbose name, and help text. The configuration matches the Chapter model's corresponding field.backend/tests/apps/owasp/api/internal/nodes/chapter_test.py (1)
10-27: LGTM: Meta configuration test is comprehensive.The test properly verifies that all expected fields are present in the ChapterNode schema.
backend/tests/apps/owasp/management/commands/owasp_aggregate_contributions_test.py (9)
34-57: LGTM!The fixtures are well-structured and provide appropriate mock data for testing both chapters and projects with their associated repositories.
58-83: LGTM!The test properly verifies date aggregation logic, including handling of same-day contributions and skipping None values.
84-120: LGTM!The test comprehensively covers chapter contribution aggregation across all contribution types (commits, issues, PRs, releases) and verifies the daily count aggregation.
121-159: LGTM!The test properly verifies project contribution aggregation across multiple repositories and all contribution types.
160-178: LGTM!Both edge case tests properly verify that entities without repositories return empty contribution maps.
179-194: LGTM!The test properly verifies command execution for chapters, including contribution data assignment and bulk save operation.
195-210: LGTM!The test properly verifies command execution for projects, mirroring the chapter test structure.
211-243: LGTM!The test properly verifies that the command processes both chapters and projects when entity_type="both", with appropriate use of context managers for multiple patches.
276-296: LGTM!The test properly verifies that the custom days parameter correctly calculates the start_date, with appropriate tolerance for test execution time.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/apps/owasp/management/commands/owasp_aggregate_contributions.py (1)
245-255: Avoid N+1 queries for projects by prefetchingrepositoriesand selectingowasp_repository
aggregate_project_contributionscallsproject.repositories.all()and accessesproject.owasp_repositoryfor each project. Becauseproject_querysetdoes not currently useprefetch_related/select_related, this results in N+1 queries as the number of projects grows.Optimize the queryset before evaluating it:
- if entity_type in ["project", "both"]: - project_queryset = Project.objects.filter(is_active=True) - - if key: - project_queryset = project_queryset.filter(key=key) - - if offset: - project_queryset = project_queryset[offset:] - - projects = list(project_queryset) + if entity_type in ["project", "both"]: + project_queryset = Project.objects.filter(is_active=True) + + if key: + project_queryset = project_queryset.filter(key=key) + + if offset: + project_queryset = project_queryset[offset:] + + project_queryset = project_queryset.prefetch_related( + "repositories", + ).select_related("owasp_repository") + + projects = list(project_queryset)This keeps existing filtering/offset semantics while collapsing the per‑project relationship lookups into a small, fixed number of queries.
🧹 Nitpick comments (1)
backend/apps/owasp/management/commands/owasp_aggregate_contributions.py (1)
48-67: Make_aggregate_contribution_datesresilient to date/datetime variants
date_value.date().isoformat()assumes adatetimeinstance. If this helper is ever reused with aDateField(or any non-datetimetype), it will raise at runtime. You could defensively handle bothdateanddatetimehere to future‑proof the helper without changing current behavior.Example:
- for date_value in dates: - if date_value: - date_key = date_value.date().isoformat() - contribution_map[date_key] = contribution_map.get(date_key, 0) + 1 + for date_value in dates: + if not date_value: + continue + + if hasattr(date_value, "date"): + date_obj = date_value.date() + else: + date_obj = date_value + + date_key = date_obj.isoformat() + contribution_map[date_key] = contribution_map.get(date_key, 0) + 1This keeps current semantics for
DateTimeFieldand won’t break if a future caller passes a plaindate.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/apps/owasp/management/commands/owasp_aggregate_contributions.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/apps/owasp/management/commands/owasp_aggregate_contributions.py (1)
backend/apps/github/models/commit.py (1)
Commit(11-110)
🔇 Additional comments (1)
backend/apps/owasp/management/commands/owasp_aggregate_contributions.py (1)
68-200: Aggregation logic for chapters and projects looks solidThe chapter/project aggregation methods correctly:
- Guard on missing repositories (chapter) and empty repository sets (project).
- Reuse
_aggregate_contribution_datesfor commits, issues, PRs, and non‑draft releases.- Use
created_at__gte/published_at__gtewith a computedstart_date.- Return a
{ "YYYY-MM-DD": count }map suitable for the JSONField.This matches the described backend aggregation behavior and keeps the implementation tidy and testable.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/tests/apps/owasp/management/commands/owasp_aggregate_contributions_test.py (1)
12-32: Consider adding docstrings to magic methods for better maintainability.The MockQuerySet class provides a clear purpose, but the magic methods (
__iter__,__getitem__,__len__) lack docstrings. While this is a test helper, adding brief docstrings would improve maintainability.Example:
def __iter__(self): + """Iterate over items.""" return iter(self._items) def __getitem__(self, key): + """Support index and slice access.""" if isinstance(key, slice): return MockQuerySet(self._items[key]) return self._items[key] def __len__(self): + """Return number of items.""" return len(self._items)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/tests/apps/owasp/api/internal/nodes/chapter_test.py(1 hunks)backend/tests/apps/owasp/management/commands/owasp_aggregate_contributions_test.py(1 hunks)frontend/src/app/chapters/[chapterKey]/page.tsx(3 hunks)frontend/src/app/projects/[projectKey]/page.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/app/chapters/[chapterKey]/page.tsx
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-06-20T16:12:59.256Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 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.
Applied to files:
frontend/src/app/projects/[projectKey]/page.tsx
📚 Learning: 2025-07-13T11:29:25.245Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 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.
Applied to files:
frontend/src/app/projects/[projectKey]/page.tsx
📚 Learning: 2025-06-20T16:12:59.256Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 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.
Applied to files:
frontend/src/app/projects/[projectKey]/page.tsx
🧬 Code graph analysis (1)
backend/tests/apps/owasp/management/commands/owasp_aggregate_contributions_test.py (2)
backend/apps/owasp/management/commands/owasp_aggregate_contributions.py (5)
Command(16-271)_aggregate_contribution_dates(48-66)aggregate_chapter_contributions(68-131)aggregate_project_contributions(133-200)handle(202-271)backend/apps/owasp/api/internal/queries/chapter.py (1)
chapter(14-19)
🪛 Ruff (0.14.5)
backend/tests/apps/owasp/management/commands/owasp_aggregate_contributions_test.py
18-18: Missing docstring in magic method
(D105)
21-21: Missing docstring in magic method
(D105)
30-30: Missing docstring in magic method
(D105)
🔇 Additional comments (11)
frontend/src/app/projects/[projectKey]/page.tsx (3)
4-8: LGTM! Imports are appropriate for the contribution heatmap feature.The new FontAwesome icons and ContributionHeatmap component are correctly imported to support the contribution activity visualization.
Also applies to: 14-14, 25-25
98-104: LGTM! Date range calculation is correct.The 1-year lookback window calculation and ISO date formatting are implemented correctly for the heatmap date range.
148-221: LGTM! Heatmap section rendering is well-implemented.The conditional rendering properly checks for data existence, stats cards use safe property access with optional chaining (
contributionStats?.commits ?? 0), and the ContributionHeatmap component receives all required props correctly.backend/tests/apps/owasp/api/internal/nodes/chapter_test.py (3)
1-33: LGTM! Test structure is well-organized.The test class setup, field name verification, and helper method follow good testing patterns.
34-52: LGTM! Field type assertions are correct.The tests properly verify field existence and types for primitive fields (str, bool) using identity checks.
54-57: Fix incomplete type assertion for contribution_data field.The assertion
field.type is objectdoesn't properly verify the JSON scalar type. Without explicit configuration, strawberry-django does not have special handling for Django's JSONField and requires mapping it to a proper GraphQL scalar like strawberry.scalars.JSON or typing.Any.Since no field_type_map configuration exists in the codebase, the field defaults to Python's base
objecttype, which is not a proper GraphQL scalar. Update the assertion to verify the correct JSON scalar type, following the pattern of other field type assertions in lines 34-52.Verify:
- Whether
contribution_datashould map tostrawberry.scalars.JSONortyping.Any- Add field_type_map configuration if needed
- Update test assertion accordingly
backend/tests/apps/owasp/management/commands/owasp_aggregate_contributions_test.py (5)
35-56: LGTM! Test fixtures are well-structured.The fixtures properly mock Chapter and Project instances with repository relationships, providing clean test setup.
58-82: LGTM! Helper method test covers key scenarios.The test properly verifies date aggregation logic including same-day counting and None value handling.
84-158: LGTM! Aggregation tests comprehensively verify contribution counting.Both chapter and project aggregation tests properly mock all contribution types and verify correct daily aggregation.
160-177: LGTM! Edge case tests ensure graceful handling of missing repositories.The tests properly verify that entities without repositories return empty contribution maps.
179-298: LGTM! Command handle tests provide comprehensive coverage.The tests verify all command execution paths including entity type filtering, key-based filtering, offset pagination, and custom time windows. The offset test now includes proper assertions verifying the pagination behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/src/app/projects/[projectKey]/page.tsx (1)
107-125: Backend should provide per-type contribution data instead of frontend estimation.The frontend uses arbitrary percentages (60% commits, 23% issues, 15% PRs) to estimate contribution breakdown from the aggregated total. This produces inaccurate statistics that don't reflect the actual contribution distribution.
As noted in the previous review, the backend aggregation logic should store per-type daily counts instead of a single aggregated count, enabling accurate statistics without estimation.
🧹 Nitpick comments (1)
frontend/src/app/projects/[projectKey]/page.tsx (1)
159-208: Consider more semantically appropriate icons.The icon choices are functional but could be refined:
- Line 174:
faCodeBranchfor PRs is acceptable but not ideal—branches are a precursor to PRs rather than representing the PR itself.- Line 198:
faCodeMergefor "Total" is semantically incorrect—merge represents a specific action, not an aggregate count. ConsiderfaChartLineor a more generic aggregation icon.These are minor clarity improvements and don't affect functionality.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/app/chapters/[chapterKey]/page.tsx(3 hunks)frontend/src/app/projects/[projectKey]/page.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/app/chapters/[chapterKey]/page.tsx
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-06-20T16:12:59.256Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 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.
Applied to files:
frontend/src/app/projects/[projectKey]/page.tsx
📚 Learning: 2025-07-13T11:29:25.245Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 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.
Applied to files:
frontend/src/app/projects/[projectKey]/page.tsx
📚 Learning: 2025-06-20T16:12:59.256Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 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.
Applied to files:
frontend/src/app/projects/[projectKey]/page.tsx
🔇 Additional comments (2)
frontend/src/app/projects/[projectKey]/page.tsx (2)
98-103: Date range calculation is correct.The 1-year lookback calculation using
toISOString().split('T')[0]is a standard approach for deriving YYYY-MM-DD format dates and is appropriate for the heatmap date range.
148-221: Well-structured contribution activity section.The implementation effectively handles:
- Defensive conditional rendering checking both existence and length
- Responsive grid layout for stat tiles
- Consistent null/undefined checking with
typeofguards and ternary operators- Proper integration of the ContributionHeatmap component
- Clean separation between DetailsCard and the new contribution activity block
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
frontend/src/app/projects/[projectKey]/page.tsx (1)
105-125: Frontend estimation of contribution breakdown remains inaccurate.This is the same issue identified in the previous review: the hardcoded percentages (60% commits, 23% issues, 15% PRs) produce inaccurate statistics that don't reflect actual contribution distribution.
The backend should store per-type contribution counts instead of aggregated totals. Refer to the previous review comment for the recommended solution.
🧹 Nitpick comments (1)
backend/tests/apps/owasp/api/internal/nodes/chapter_test.py (1)
54-58: Consider usingisinstanceor direct type comparison for more robust type checking.The test verifies the field type by comparing
__class__.__name__to the string"ScalarWrapper", which is fragile and will break if the class is renamed or if the import path changes.Consider a more robust approach:
def test_resolve_contribution_data(self): field = self._get_field_by_name("contribution_data") assert field is not None - # JSONField is represented as a Strawberry ScalarWrapper for JSON type - assert field.type.__class__.__name__ == "ScalarWrapper" + # JSONField is represented as a Strawberry ScalarWrapper for JSON type + from strawberry.types.types import StrawberryType + assert isinstance(field.type, StrawberryType) or hasattr(field.type, '__strawberry_definition__')Alternatively, if you need to check specifically for JSON scalar, import the actual type:
# At top of file from strawberry.scalars import JSON def test_resolve_contribution_data(self): field = self._get_field_by_name("contribution_data") assert field is not None # Check if it's the JSON scalar type or wrapped version assert field.type is JSON or (hasattr(field.type, 'of_type') and field.type.of_type is JSON)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/apps/owasp/management/commands/owasp_aggregate_contributions.py(1 hunks)backend/tests/apps/owasp/api/internal/nodes/chapter_test.py(1 hunks)backend/tests/apps/owasp/management/commands/owasp_aggregate_contributions_test.py(1 hunks)frontend/src/app/chapters/[chapterKey]/page.tsx(3 hunks)frontend/src/app/projects/[projectKey]/page.tsx(3 hunks)frontend/src/components/ContributionHeatmap.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/apps/owasp/management/commands/owasp_aggregate_contributions.py
- frontend/src/app/chapters/[chapterKey]/page.tsx
- backend/tests/apps/owasp/management/commands/owasp_aggregate_contributions_test.py
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-07-13T11:29:25.245Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 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.
Applied to files:
frontend/src/app/projects/[projectKey]/page.tsx
📚 Learning: 2025-06-20T16:12:59.256Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 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.
Applied to files:
frontend/src/app/projects/[projectKey]/page.tsx
📚 Learning: 2025-06-20T16:12:59.256Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 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.
Applied to files:
frontend/src/app/projects/[projectKey]/page.tsx
📚 Learning: 2025-06-21T12:21:32.372Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 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.
Applied to files:
frontend/src/components/ContributionHeatmap.tsx
🧬 Code graph analysis (1)
backend/tests/apps/owasp/api/internal/nodes/chapter_test.py (1)
backend/apps/owasp/api/internal/queries/chapter.py (1)
chapter(14-19)
🔇 Additional comments (2)
frontend/src/app/projects/[projectKey]/page.tsx (1)
148-229: LGTM: Contribution activity UI implementation is well-structured.The conditional rendering, layout, accessibility with icons, and integration with ContributionHeatmap component are all properly implemented. The UI correctly handles the case when contribution data exists and displays stats alongside the heatmap visualization.
Note: The accuracy of the displayed stats depends on resolving the estimation issue flagged separately (lines 105-125).
frontend/src/components/ContributionHeatmap.tsx (1)
250-289: Variant implementation is well-designed.The compact and default variant handling through CSS classes, dynamic sizing, and responsive behavior is implemented correctly. The approach maintains code clarity while supporting both use cases effectively.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
frontend/src/app/projects/[projectKey]/page.tsx (1)
101-121: Same hardcoded percentage estimation issue as chapter page.This code uses the same arbitrary percentage breakdown (60%, 23%, 15%) to estimate contribution statistics. The root cause is identical: the backend aggregates all contribution types into single daily counts, forcing frontend estimation.
Apply the same fix as suggested for the chapter page (lines 72-92 in
frontend/src/app/chapters/[chapterKey]/page.tsx):
- Modify backend to store per-type daily counts
- Update frontend to sum actual per-type values
- Coordinate changes across backend models, GraphQL, and frontend types
🧹 Nitpick comments (1)
frontend/src/components/ContributionHeatmap.tsx (1)
167-172: Consider using date-fns for more robust date parsing.The string concatenation (
date + 'T00:00:00Z') works but is fragile if the date format changes. Since date-fns is already a project dependency, consider usingparseISOfor safer parsing.Apply this diff:
+import { parseISO, format } from 'date-fns' +import { formatInTimeZone } from 'date-fns-tz' const count = data.y const date = data.date - // Parse date as UTC to match data format - const formattedDate = new Date(date + 'T00:00:00Z').toLocaleDateString('en-US', { - weekday: 'short', - month: 'short', - day: 'numeric', - timeZone: 'UTC', - }) + // Parse ISO date and format in UTC + const formattedDate = formatInTimeZone( + parseISO(date), + 'UTC', + 'EEE, MMM d' + )
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/apps/owasp/management/commands/owasp_aggregate_contributions.py(1 hunks)backend/tests/apps/owasp/management/commands/owasp_aggregate_contributions_test.py(1 hunks)frontend/src/app/chapters/[chapterKey]/page.tsx(2 hunks)frontend/src/app/projects/[projectKey]/page.tsx(2 hunks)frontend/src/components/ContributionHeatmap.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/tests/apps/owasp/management/commands/owasp_aggregate_contributions_test.py
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-06-20T16:12:59.256Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 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.
Applied to files:
frontend/src/app/projects/[projectKey]/page.tsxfrontend/src/app/chapters/[chapterKey]/page.tsx
📚 Learning: 2025-06-20T16:12:59.256Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 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.
Applied to files:
frontend/src/app/projects/[projectKey]/page.tsx
📚 Learning: 2025-06-21T12:21:32.372Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 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.
Applied to files:
frontend/src/components/ContributionHeatmap.tsx
🧬 Code graph analysis (3)
frontend/src/app/projects/[projectKey]/page.tsx (1)
frontend/src/components/ContributionStats.tsx (1)
ContributionStats(22-82)
frontend/src/app/chapters/[chapterKey]/page.tsx (1)
frontend/src/components/ContributionStats.tsx (1)
ContributionStats(22-82)
backend/apps/owasp/management/commands/owasp_aggregate_contributions.py (3)
backend/apps/github/models/commit.py (1)
Commit(11-110)backend/tests/apps/github/models/issue_test.py (1)
issue(26-28)backend/tests/apps/owasp/management/commands/owasp_aggregate_contributions_test.py (3)
filter(28-30)select_related(32-34)prefetch_related(36-38)
🔇 Additional comments (2)
frontend/src/components/ContributionHeatmap.tsx (1)
9-294: Excellent refactoring improves testability and maintainability.The extraction of
generateHeatmapSeriesandgetChartOptionsinto separate helpers makes the code more testable and easier to understand. The addition of thevariantprop provides good flexibility for different UI contexts.backend/apps/owasp/management/commands/owasp_aggregate_contributions.py (1)
227-281: Query optimizations and bulk save usage are well implemented.The use of
select_related("owasp_repository")for chapters andselect_related("owasp_repository").prefetch_related("repositories")for projects prevents N+1 queries. Thebulk_saveapproach efficiently updates multiple records. The helper method separation improves readability.
backend/apps/owasp/management/commands/owasp_aggregate_contributions.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
frontend/src/components/ContributionStats.tsx (2)
67-78: Consider a more semantically appropriate icon for the Total metric.The
faCodeMergeicon specifically represents code/PR merging operations, which may confuse users since "Total" represents the sum of all contribution types, not just merges. Consider alternatives likefaChartBar,faCalculator, orfaListOlthat better convey aggregation or totals.Apply this diff to use a more appropriate icon:
import { faChartLine, faCode, faCodeBranch, - faCodeMerge, + faChartBar, faExclamationCircle, } from '@fortawesome/free-solid-svg-icons'Then update the icon reference:
<div className="flex items-center gap-2"> <FontAwesomeIcon - icon={faCodeMerge} + icon={faChartBar} className="h-5 w-5 text-gray-600 dark:text-gray-400" />
33-79: Consider adding accessibility attributes for screen readers.The statistics grid lacks semantic structure and ARIA labels. While the visual presentation is clear, screen reader users would benefit from explicit role and label attributes.
Apply this diff to improve accessibility:
- <div className="mb-6 grid grid-cols-2 gap-4 sm:grid-cols-4"> + <div className="mb-6 grid grid-cols-2 gap-4 sm:grid-cols-4" role="list" aria-label="Contribution statistics"> - <div className="flex items-center gap-2"> + <div className="flex items-center gap-2" role="listitem"> <FontAwesomeIcon icon={faCode} className="h-5 w-5 text-gray-600 dark:text-gray-400" /> - <div> + <div aria-label={`${formatNumber(stats?.commits)} commits`}> <p className="text-sm font-medium text-gray-500 dark:text-gray-400">Commits</p>Apply similar changes to the other three metric blocks (PRs, Issues, Total).
frontend/src/components/ContributionHeatmap.tsx (2)
263-288: Consider refactoring dynamic CSS generation for maintainability.The current approach uses template strings to generate CSS classes and applies transforms via inline styles. While functional, this approach has some drawbacks:
- Dynamic class names make it harder to debug and inspect styles
- Scaling transforms on the canvas can cause subpixel rendering issues
- The fixed 1200px width on default variant relies on scaling rather than proper responsive layout
Consider:
- Define static CSS classes for each variant in a CSS module or styled component
- Use CSS Grid or Flexbox with proper responsive breakpoints instead of transform scaling
- Let ApexCharts handle responsive sizing via the responsive options array
This would improve maintainability and avoid potential rendering artifacts from CSS transforms.
289-297: Add accessibility attributes for the heatmap chart.The ApexCharts Chart component doesn't support ARIA attributes as direct props. To make the heatmap accessible to screen readers, wrap it in a container with appropriate ARIA attributes.
Apply this diff to add accessibility:
- <div className={`heatmap-container-${isCompact ? 'compact' : 'default'}`}> + <div + className={`heatmap-container-${isCompact ? 'compact' : 'default'}`} + role="img" + aria-label={`${title || 'Contribution'} heatmap showing activity from ${new Date(startDate).toLocaleDateString()} to ${new Date(endDate).toLocaleDateString()}`} + > <ChartBased on learnings
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/components/ContributionHeatmap.tsx(2 hunks)frontend/src/components/ContributionStats.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-21T12:21:32.372Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 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.
Applied to files:
frontend/src/components/ContributionHeatmap.tsx
🔇 Additional comments (7)
frontend/src/components/ContributionStats.tsx (3)
1-8: LGTM!The imports are correctly structured for FontAwesome icons and the React component.
10-20: LGTM!The interface design is clean with appropriate use of optional fields and readonly modifiers.
22-25: LGTM!The formatNumber helper correctly handles undefined values and provides locale-aware number formatting for better UX.
Note: The
Readonly<ContributionStatsProps>wrapper is redundant since the interface already uses readonly modifiers, but it's harmless.frontend/src/components/ContributionHeatmap.tsx (4)
1-7: LGTM!The dynamic import with
ssr: falseis the correct approach for ApexCharts in Next.js, ensuring client-side-only rendering.
10-66: LGTM!The date calculation logic is well-implemented:
- Correctly finds the first Monday to start the heatmap grid
- Properly adjusts day indices for the heatmap layout
- Manual date formatting avoids timezone issues
- Range check ensures contributions are only counted within the specified date range
- Week numbering increments correctly on Mondays
- Reversing the series ensures Monday appears at the top
69-224: LGTM!The chart options are well-configured:
- UTC date parsing matches the YYYY-MM-DD format used in data
- Color scale ranges provide good visual differentiation
- Custom tooltip styling correctly uses inline styles with !important to override ApexCharts defaults
- Proper dark/light mode support
Note: The pluralization logic on line 179 is simple (
${unit}s) and works fine for "contribution", but won't handle irregular plurals. This is acceptable for the current use case.
226-253: LGTM!The component logic is well-structured:
- Clear prop interface with variant support
- Proper memoization of series and options with correct dependencies
- Theme integration for dark mode support
mrkeshav-05
force-pushed
the
feature/contribution-heatmap-chapter-project
branch
from
dac4c4c to
bd2bc94
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/src/app/chapters/[chapterKey]/page.tsx (1)
72-92: Hardcoded contribution-type percentages still produce misleading statsThe breakdown for commits/issues/PRs is derived from fixed 60%/23%/15% splits of the total, even though the backend only exposes an undifferentiated per-day count. This makes the per-type numbers look precise when they are actually guesses and they may not even sum to the displayed total.
Consider either:
- Dropping the per-type breakdown for now and having
ContributionStatsfocus on totals (e.g., total contributions, active days) until the backend stores per-type counts, or- Extending the aggregation to persist per-type data (e.g.,
{date: {commits, issues, pullRequests, releases}}) and updating this logic to sum real values instead of estimating.Also applies to: 112-119
🧹 Nitpick comments (3)
backend/tests/apps/owasp/api/internal/nodes/chapter_test.py (1)
10-58: ChapterNode tests are good; JSON ScalarWrapper check is a bit brittleThe tests nicely verify that
contribution_datais exposed on ChapterNode, buttest_resolve_contribution_datarelies onfield.type.__class__.__name__ == "ScalarWrapper", which couples the test to Strawberry’s internal class name and may break on library upgrades even if the JSON mapping is still correct.If you want this test to be more future-proof, consider asserting against the JSON scalar more generically (for example, by checking for expected
serialize/parse_valueattributes, or by comparing with the configured JSON scalar type if you expose it) instead of depending on the concrete class name.backend/tests/apps/owasp/management/commands/owasp_aggregate_contributions_test.py (1)
190-270: Aggregation command tests are solid; specific-key test could assert more stronglyThe mocking strategy and coverage for helper, chapter/project paths, offset, and custom days are thorough. In
test_handle_with_specific_key, though, the final assertion only checks thatChapter.objects.filterwas called, not that the queryset’s subsequent.filter(key=...)was invoked, so it doesn’t fully validate the behavior described in the docstring.You could strengthen this by instrumenting
MockQuerySet.filterto recordkwargsand asserting that one of the calls includeskey="www-chapter-test", or by explicitly asserting onmock_chapter_model.objects.filter.call_args_listto ensure the key-based filter path is exercised.backend/apps/owasp/management/commands/owasp_aggregate_contributions.py (1)
202-281: Command orchestration and query patterns look solid; batching is a future optimization
handleand the_process_chapters/_process_projectshelpers:
- Respect
--entity-type,--days,--key, and--offset.- Use
timezone.now() - timedelta(days=days)consistently for the lower bound.- Apply
select_related("owasp_repository")andprefetch_related("repositories")before materializing lists, which avoids N+1s.- Persist only
contribution_dataviabulk_save, which is efficient.If you ever expect a very large number of chapters/projects, you might consider processing in smaller batches (e.g., via queryset
iterator()+ chunked bulk saves) to keep memory bounded, but for current usage this is an optional optimization, not a blocker.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
frontend/src/types/__generated__/chapterQueries.generated.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/graphql.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/projectQueries.generated.tsis excluded by!**/__generated__/**
📒 Files selected for processing (19)
backend/apps/owasp/api/internal/nodes/chapter.py(1 hunks)backend/apps/owasp/api/internal/nodes/project.py(1 hunks)backend/apps/owasp/management/commands/owasp_aggregate_contributions.py(1 hunks)backend/apps/owasp/migrations/0066_chapter_contribution_data_project_contribution_data.py(1 hunks)backend/apps/owasp/models/chapter.py(1 hunks)backend/apps/owasp/models/project.py(1 hunks)backend/tests/apps/owasp/api/internal/nodes/chapter_test.py(1 hunks)backend/tests/apps/owasp/api/internal/nodes/project_test.py(1 hunks)backend/tests/apps/owasp/management/commands/owasp_aggregate_contributions_test.py(1 hunks)frontend/src/app/board/[year]/candidates/page.tsx(2 hunks)frontend/src/app/chapters/[chapterKey]/page.tsx(2 hunks)frontend/src/app/projects/[projectKey]/page.tsx(2 hunks)frontend/src/components/ContributionHeatmap.tsx(2 hunks)frontend/src/components/ContributionStats.tsx(1 hunks)frontend/src/components/SponsorCard.tsx(1 hunks)frontend/src/server/queries/chapterQueries.ts(1 hunks)frontend/src/server/queries/projectQueries.ts(1 hunks)frontend/src/types/chapter.ts(1 hunks)frontend/src/types/project.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- frontend/src/app/projects/[projectKey]/page.tsx
- frontend/src/server/queries/chapterQueries.ts
- frontend/src/components/ContributionStats.tsx
- backend/apps/owasp/api/internal/nodes/project.py
- backend/apps/owasp/migrations/0066_chapter_contribution_data_project_contribution_data.py
- backend/tests/apps/owasp/api/internal/nodes/project_test.py
- frontend/src/types/chapter.ts
- backend/apps/owasp/models/chapter.py
- frontend/src/app/board/[year]/candidates/page.tsx
- frontend/src/types/project.ts
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-07-11T15:57:56.648Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 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.
Applied to files:
backend/apps/owasp/api/internal/nodes/chapter.py
📚 Learning: 2025-07-13T05:55:46.436Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 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.
Applied to files:
backend/apps/owasp/api/internal/nodes/chapter.py
📚 Learning: 2025-06-20T16:12:59.256Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 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.
Applied to files:
frontend/src/app/chapters/[chapterKey]/page.tsx
📚 Learning: 2025-06-20T16:12:59.256Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 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.
Applied to files:
frontend/src/app/chapters/[chapterKey]/page.tsx
📚 Learning: 2025-06-21T12:21:32.372Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 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.
Applied to files:
frontend/src/components/ContributionHeatmap.tsx
🧬 Code graph analysis (3)
frontend/src/app/chapters/[chapterKey]/page.tsx (1)
frontend/src/components/ContributionStats.tsx (1)
ContributionStats(22-82)
backend/tests/apps/owasp/management/commands/owasp_aggregate_contributions_test.py (1)
backend/apps/owasp/management/commands/owasp_aggregate_contributions.py (5)
Command(16-281)_aggregate_contribution_dates(48-66)aggregate_chapter_contributions(68-131)aggregate_project_contributions(133-200)handle(202-225)
backend/apps/owasp/management/commands/owasp_aggregate_contributions.py (2)
backend/apps/github/models/commit.py (1)
Commit(11-110)backend/tests/apps/owasp/management/commands/owasp_aggregate_contributions_test.py (3)
filter(28-30)select_related(32-34)prefetch_related(36-38)
🪛 GitHub Check: SonarCloud Code Analysis
frontend/src/components/ContributionHeatmap.tsx
[warning] 64-64: Move this array "reverse" operation to a separate statement or replace it with "toReversed".
🔇 Additional comments (10)
frontend/src/components/SponsorCard.tsx (1)
4-4: SponsorCard spacing tweak is safe and containedRemoving the bottom margin from the outer div only tightens vertical spacing; no behavioral or accessibility impact.
frontend/src/server/queries/projectQueries.ts (1)
92-92: GET_PROJECT_DATA now fetches heatmap data as expectedAdding
contributionDatato the project query aligns with the new ProjectNode field and frontend heatmap/stats usage; no structural issues spotted.backend/apps/owasp/api/internal/nodes/chapter.py (1)
18-30: ChapterNode correctly exposes contribution_dataIncluding
"contribution_data"in the ChapterNode fields list cleanly surfaces the new JSONField to GraphQL clients and matches the model and tests.backend/apps/owasp/models/project.py (1)
100-105: Project.contribution_data JSONField is well-definedThe JSONField uses
default=dict, is optional (blank=True), and documents the YYYY-MM-DD → count mapping clearly; this matches the aggregation command’s expected shape.frontend/src/app/chapters/[chapterKey]/page.tsx (2)
65-71: Heatmap date range + guarded rendering look solidComputing a 1-year
[startDate, endDate]window and only rendering the heatmap/stats whenchapter.contributionDatahas entries matches the existing pattern used to protect HealthMetrics from empty data and keeps the component usage safe. Based on learnings.Also applies to: 108-124
94-107: Wrapping DetailsCard and new contribution section in a fragment is cleanReturning a fragment that contains the existing
DetailsCardplus the optional contribution panel keeps the page component structure simple and doesn’t alter the DetailsCard behavior.frontend/src/components/ContributionHeatmap.tsx (3)
68-224: Chart options and tooltip wiring look consistent with Apex heatmap usageThe extracted
getChartOptionskeeps the heatmap configuration centralized and the tooltip’scustomcallback correctly relies on the{ x, y, date }structure produced bygenerateHeatmapSeries(including dark/light colors and pluralizedunit). From a quick read, the axis/grid/tooltip flags all look aligned with a compact, non-interactive heatmap.No changes needed here from my side; just sanity‑check the tooltip rendering in both themes visually.
226-296: Props, memoization, and compact/default layout handling look goodThe
ContributionHeatmapprops shape (contributionData,startDate,endDate, optionaltitle,unit,variant) is clear,useMemocorrectly scopes recomputation to data and theme changes, and the compact vs default variant toggles are cleanly expressed via the container class and width/height differences.The inline
<style>block keyed offisCompactandisDarkModeis a pragmatic way to tweak Apex’ internals without touching the component API. No issues to flag here.
9-66: Clarify weekday ordering and avoid mutatingseriesin-placeThe review comment is valid. The code exhibits two issues:
Ordering contradiction:
dayNamesstarts with Monday at index 0. Afterseries.reverse()is called, Monday moves to index 6 and Sunday to index 0. ApexCharts renders series[0] as the top row, so this reversal places Sunday at the top—contradicting the comment that claims "Monday is at the top and Sunday at the bottom." The developer must clarify whether Monday or Sunday should be the top row and adjust the code or comment accordingly.In-place mutation:
reverse()mutates the array, which Sonar correctly flags. The suggested fix using[...series].reverse()avoids this mutation and is a sound improvement.The suggested diff is correct:
- const reversedSeries = series.reverse() - return { heatmapSeries: reversedSeries } + const heatmapSeries = [...series].reverse() + return { heatmapSeries }However, before applying this, the developer should verify the intended visual layout and confirm whether the
reverse()call itself is necessary or should be removed entirely.backend/apps/owasp/management/commands/owasp_aggregate_contributions.py (1)
48-200: Aggregation helpers match the intended{date: count}contractThe
_aggregate_contribution_dateshelper and theaggregate_chapter_contributions/aggregate_project_contributionsmethods correctly:
- Count by
created_atfor commits/issues/PRs andpublished_atfor non‑draft releases.- Normalize to
YYYY-MM-DDviadate().isoformat().- Guard against missing timestamps and missing repositories.
- For projects, aggregate across both
repositoriesandowasp_repositoryusingrepository_id__in, which avoids double-counting even if IDs repeat.This aligns with the backend spec of storing a flat
{ "YYYY-MM-DD": count }map on the models. No changes needed here.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
backend/apps/owasp/management/commands/owasp_aggregate_contributions.py (1)
48-67: Helper cleanly centralizes daily aggregation; per-type breakdown remains a future enhancementThe
_aggregate_contribution_dateshelper is concise and reusable, and the use of.values_list(...).date().isoformat()gives a stableYYYY-MM-DD -> countmap that fits the current JSONField contract.One trade-off (also raised earlier) is that this structure only tracks a total count per day, so any UI that wants per-type (commits/issues/PRs/releases) statistics has to approximate from totals. Given the current spec explicitly calls for
{ "YYYY-MM-DD": count, ... }, this is acceptable, but if you later need accurate per-type metrics you’ll need to evolvecontribution_mapto a nested{date: {type: count}}shape and propagate that through models, GraphQL, and the frontend.
🧹 Nitpick comments (3)
frontend/src/components/ContributionHeatmap.tsx (1)
179-179: Pluralization logic is English-only.The simple pluralization (
count !== 1 ?${unit}s: unit) will not work correctly for all English words (e.g., "activity" → "activitys") or for internationalized content. If i18n is planned, consider using a proper pluralization library or passing plural forms as separate props.backend/apps/owasp/management/commands/owasp_aggregate_contributions.py (2)
133-200: Project aggregation correctly merges repositories; consider deduping IDs defensivelyThe project aggregation correctly combines
project.repositoriesand an optionalproject.owasp_repository, then queries byrepository_id__in. That matches the requirement to aggregate across all project repositories.If there’s any chance
project.owasp_repositorycan also appear inproject.repositories, you might want to dedupe IDs before passing them to the__infilter (purely for clarity and to avoid redundant parameters):- repository_ids = [repo.id for repo in repositories if repo] + repository_ids = list({repo.id for repo in repositories if repo})Functionally this shouldn’t change results, but it makes the intent explicit.
31-35: Aligndayswindow with date-based buckets to avoid truncating the first day
start_dateis computed astimezone.now() - timedelta(days=days)(a datetime), while aggregation buckets contributions bydate_value.date().isoformat(). With this combination, events that occurred exactlydaysdays ago but earlier in that day (e.g., morning of the first day) will be excluded even though they share the same calendar date as the first bucket.For a date-based heatmap, you may get more intuitive results by aligning the window to whole days, e.g., either:
- Normalize
start_dateto midnight before filtering, or- Filter on the date component (
created_at__date__gte/published_at__date__gte) instead of raw datetimes.This isn’t a blocker, but tightening this would make the “last N days” behavior match the calendar dates you expose in
contribution_data.Also applies to: 209-215
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/apps/owasp/management/commands/owasp_aggregate_contributions.py(1 hunks)frontend/src/components/ContributionHeatmap.tsx(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-21T12:21:32.372Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 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.
Applied to files:
frontend/src/components/ContributionHeatmap.tsx
🧬 Code graph analysis (1)
backend/apps/owasp/management/commands/owasp_aggregate_contributions.py (2)
backend/apps/github/models/commit.py (1)
Commit(11-110)backend/tests/apps/owasp/management/commands/owasp_aggregate_contributions_test.py (3)
filter(28-30)select_related(32-34)prefetch_related(36-38)
🪛 GitHub Check: SonarCloud Code Analysis
frontend/src/components/ContributionHeatmap.tsx
[warning] 64-64: Move this array "reverse" operation to a separate statement or replace it with "toReversed".
🔇 Additional comments (3)
frontend/src/components/ContributionHeatmap.tsx (1)
226-233: Props interface looks good.The interface is well-structured, and the removal of the unused
statsprop addresses the previous review feedback.backend/apps/owasp/management/commands/owasp_aggregate_contributions.py (2)
21-47: CLI arguments are well-structured and match the command’s responsibilitiesThe argument set (
entity-type,days,key,offset) is clear, predictable, and aligns with the intended chapter/project/both use cases. No issues from a usability or correctness standpoint.
68-131: Chapter aggregation logic is consistent and filters the right contribution sourcesThe chapter path looks solid: you short‑circuit when there’s no
owasp_repository, aggregate commits/issues/PRs bycreated_at >= start_date, and releases bypublished_at >= start_datewhile excluding drafts. Using the shared helper keeps behavior uniform across contribution types.
backend/apps/owasp/management/commands/owasp_aggregate_contributions.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
frontend/src/components/ContributionHeatmap.tsx (3)
152-179: Harden tooltip date parsing against malformed inputThe tooltip still builds a
Datefromdate + 'T00:00:00Z'. This assumes a validYYYY-MM-DDstring and can silently produceInvalid Date(or off‑by‑one issues if formats ever change). Consider validating and handling failures explicitly, e.g.:- const count = data.y - const date = data.date - // Parse date as UTC to match data format - const formattedDate = new Date(date + 'T00:00:00Z').toLocaleDateString('en-US', { + const count = data.y + const date = data.date + // Parse date as UTC to match data format (YYYY-MM-DD expected) + const parsedDate = new Date(`${date}T00:00:00Z`) + const formattedDate = Number.isNaN(parsedDate.getTime()) + ? date + : parsedDate.toLocaleDateString('en-US', { weekday: 'short', month: 'short', day: 'numeric', timeZone: 'UTC', - }) + })This keeps behavior the same for valid dates while failing more safely if the input ever drifts.
271-283: Fixed 1200px chart width still risks horizontal overflow on mid‑sized screensThe combination of:
.heatmap-container-default { max-width: 100%; overflow: visible; }andwidth={isCompact ? '100%' : '1200px'}can still cause horizontal overflow and awkward scaling on tablets / smaller desktops even though the canvas is scaled to 0.85.
A more responsive approach would be to keep the chart width fluid and cap via container:
- .heatmap-container-${isCompact ? 'compact' : 'default'} { - width: 100%; - ${isCompact ? 'min-width: 380px;' : 'max-width: 100%; overflow: visible;'} - } + .heatmap-container-${isCompact ? 'compact' : 'default'} { + width: 100%; + ${isCompact ? 'min-width: 380px;' : 'max-width: 1200px; overflow: visible; margin: 0 auto;'} + } @@ - ${isCompact ? '' : '.heatmap-container-default .apexcharts-canvas { transform: scale(0.85); transform-origin: left top; }'} + ${isCompact ? '' : '.heatmap-container-default .apexcharts-canvas { transform-origin: left top; }'} @@ - height={isCompact ? '100%' : 200} - width={isCompact ? '100%' : '1200px'} + height={isCompact ? '100%' : 200} + width="100%"This preserves the intended max width while letting the chart shrink gracefully with the viewport.
Also applies to: 292-293
287-294: WrapChartwith an ARIA‑aware container for accessibility
react-apexchartsdoesn’t take ARIA props directly, so screen readers currently get no semantic information about this visualization. Per project learnings, wrap theChartin a div with appropriate ARIA attributes:- <div className={`heatmap-container-${isCompact ? 'compact' : 'default'}`}> - <Chart - options={options} - series={heatmapSeries} - type="heatmap" - height={isCompact ? '100%' : 200} - width={isCompact ? '100%' : '1200px'} - /> - </div> + <div className={`heatmap-container-${isCompact ? 'compact' : 'default'}`}> + <div + role="img" + aria-label={`${title || 'Contribution heatmap'} showing activity over time`} + > + <Chart + options={options} + series={heatmapSeries} + type="heatmap" + height={isCompact ? '100%' : 200} + width={isCompact ? '100%' : '1200px'} + /> + </div> + </div>This significantly improves accessibility without changing visual behavior. Based on learnings.
🧹 Nitpick comments (2)
frontend/src/components/ContributionHeatmap.tsx (2)
9-66: Heatmap series generation logic looks solid; minor edge-case thoughtsThe per-day loop, week labeling, and Monday‑anchored grid all look correct and should produce a consistent 7×N matrix with proper zero‑filled padding around the actual range. Only small nits:
- If
startDate > endDate, you silently return empty series; consider early‑returning or asserting to avoid confusing call‑sites.- You could add an explicit return type for
generateHeatmapSeriesfor clearer reuse and to catch accidental shape changes in future refactors.
63-65: Use[...series].reverse()to maintain compatibility with Next.js 15.5.6 defaultsArray.prototype.toReversed requires Firefox 115+, but Next.js 15.5.6 defaults to Firefox 111+, creating a compatibility gap. The current code will break in Firefox 111–114. Replace with the spread+reverse pattern to remain compatible with the stated browserslist:
const reversedSeries = [...series].reverse()This preserves immutability, works across all supported browsers, and avoids the need for polyfills.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/components/ContributionHeatmap.tsx(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-06-21T12:21:32.372Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 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.
Applied to files:
frontend/src/components/ContributionHeatmap.tsx
📚 Learning: 2025-04-30T13:41:20.846Z
Learnt from: codic-yeeshu
Repo: OWASP/Nest PR: 1444
File: frontend/src/components/NavDropDown.tsx:0-0
Timestamp: 2025-04-30T13:41:20.846Z
Learning: When implementing dropdown menus or similar interactive components, always include proper accessibility features: ARIA attributes (aria-expanded, aria-haspopup, aria-controls), keyboard navigation support (Enter, Space, Escape keys), and mechanisms to close dropdowns when clicking outside.
Applied to files:
frontend/src/components/ContributionHeatmap.tsx
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
frontend/src/components/ContributionHeatmap.tsx (2)
271-283: Dynamic CSS class generation may cause style conflicts if variant changes.The component generates CSS rules dynamically based on
isCompact, but if thevariantprop changes after initial render, the old CSS rules remain in the DOM while new ones are added. This creates duplicate style blocks and potential specificity conflicts.Consider one of these approaches:
Option 1: Use stable class names and CSS variables:
<style> {` .heatmap-container { width: 100%; } .heatmap-container.compact { min-width: 380px; } .heatmap-container.default { max-width: 100%; overflow: visible; } /* ... rest of styles with stable classes */ `} </style> <div className={`heatmap-container ${variant}`}>Option 2: Move styles to a separate CSS module that doesn't rely on runtime JavaScript.
5-7: Consider adding a loading state for the dynamic Chart import.The
Chartcomponent is dynamically imported withssr: false, which means it only renders on the client. While the import is typically fast, there's a brief moment where nothing is displayed. Adding a loading fallback improves perceived performance.Apply this diff:
const Chart = dynamic(() => import('react-apexcharts'), { ssr: false, + loading: () => ( + <div className="flex h-[200px] items-center justify-center text-sm text-gray-500"> + Loading chart... + </div> + ), })
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/components/ContributionHeatmap.tsx(2 hunks)frontend/src/components/ContributionStats.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/components/ContributionStats.tsx
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-06-21T12:21:32.372Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 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.
Applied to files:
frontend/src/components/ContributionHeatmap.tsx
📚 Learning: 2025-04-30T13:41:20.846Z
Learnt from: codic-yeeshu
Repo: OWASP/Nest PR: 1444
File: frontend/src/components/NavDropDown.tsx:0-0
Timestamp: 2025-04-30T13:41:20.846Z
Learning: When implementing dropdown menus or similar interactive components, always include proper accessibility features: ARIA attributes (aria-expanded, aria-haspopup, aria-controls), keyboard navigation support (Enter, Space, Escape keys), and mechanisms to close dropdowns when clicking outside.
Applied to files:
frontend/src/components/ContributionHeatmap.tsx
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
frontend/__tests__/unit/components/ContributionStats.test.tsx (4)
73-82: Clarify or strengthen icon verification.The comment on line 79 states "Verify specific icon data attributes" but the following assertions only check that the container and title render, not the actual icon attributes. Icon verification is more thoroughly covered in lines 324-342.
Consider either removing the misleading comment or adding actual icon attribute checks:
- // Verify specific icon data attributes - expect(screen.getByTestId('contribution-stats')).toBeInTheDocument() - expect(screen.getByText('Test Contribution Activity')).toBeInTheDocument() + // Verify icons are present + const chartIcon = icons.find(icon => icon.getAttribute('data-icon') === 'chart-line') + expect(chartIcon).toBeInTheDocument()
169-181: Strengthen negative value verification.The test verifies the component doesn't crash with negative values but doesn't assert what's actually displayed. Per the component's
formatNumberlogic, negative numbers are formatted usingtoLocaleString()and will display with minus signs.Add assertions to verify the displayed values:
render(<ContributionStats title="Negative Stats" stats={negativeStats} />) - // Component should still render, showing the negative values or handling them gracefully expect(screen.getByText('Negative Stats')).toBeInTheDocument() + expect(screen.getByText('-5')).toBeInTheDocument() + expect(screen.getByText('-3')).toBeInTheDocument() + expect(screen.getByText('-2')).toBeInTheDocument() + expect(screen.getByText('-10')).toBeInTheDocument()
183-195: Strengthen non-numeric value verification.The test verifies one valid value (total: 42) but doesn't assert how the invalid string/null/undefined values are displayed. Per the component's
formatNumberlogic, non-numeric values should display as '0'.Add assertions to verify fallback behavior:
render(<ContributionStats title="Invalid Stats" stats={invalidStats} />) expect(screen.getByText('Invalid Stats')).toBeInTheDocument() expect(screen.getByText('42')).toBeInTheDocument() // total should still work + expect(screen.getAllByText('0')).toHaveLength(3) // commits, pullRequests, issues should show 0
197-209: Verify large number formatting.The test confirms the component doesn't crash with very large numbers but doesn't verify they're formatted correctly. Per the component's
toLocaleString()usage, these should display with locale-appropriate thousand separators.Add assertions to verify formatted output:
render(<ContributionStats title="Large Stats" stats={largeStats} />) expect(screen.getByText('Large Stats')).toBeInTheDocument() - // Should not crash, even with very large numbers + // Verify MAX_SAFE_INTEGER is formatted + expect(screen.getByText('9,007,199,254,740,991')).toBeInTheDocument() + expect(screen.getByText('999,999,999')).toBeInTheDocument() + expect(screen.getByText('888,888,888')).toBeInTheDocument()frontend/__tests__/unit/components/ContributionHeatmap.test.tsx (4)
404-432: Consider verifying behavior for invalid date inputs.The tests confirm the component doesn't crash with invalid date ranges (end before start) or malformed dates, but don't verify the expected behavior. Consider adding assertions to check if the component handles these cases appropriately (e.g., swapping dates, showing an error, rendering empty).
557-562: Consider using a more stable selector for container verification.Navigating up two parent elements (
parentElement?.parentElement) is fragile and will break if the DOM structure changes. Consider adding a test-id to the container element or usingclosest()with a class/attribute selector.Example refactor:
- const container = screen.getByTestId('contribution-heatmap-chart').parentElement?.parentElement + const container = screen.getByTestId('contribution-heatmap-chart').closest('.max-w-5xl') expect(container).toHaveClass('max-w-5xl')
588-627: Consider verifying error handling behavior.These tests confirm the component doesn't crash with negative or non-numeric values, but don't verify how these edge cases are handled (e.g., treated as 0, filtered out, displayed as-is). For more thorough coverage, consider adding assertions on the expected behavior.
640-671: Performance tests don't verify memoization.These tests claim to verify memoization but only check that re-renders don't break the component. They don't actually validate that
useMemooruseCallbackprevent recomputation. To truly test memoization, you'd need to spy on the memoized functions or useReact.profilerto count renders. Consider renaming these tests to reflect what they actually verify (e.g., "handles re-renders without issues") or enhancing them with proper memoization assertions.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/__tests__/unit/components/ContributionHeatmap.test.tsx(1 hunks)frontend/__tests__/unit/components/ContributionStats.test.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-07-12T17:36:57.255Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 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.
Applied to files:
frontend/__tests__/unit/components/ContributionStats.test.tsx
📚 Learning: 2025-11-17T16:47:05.578Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:427-427
Timestamp: 2025-11-17T16:47:05.578Z
Learning: In the frontend test files for the OWASP/Nest repository, `expect(true).toBe(true)` no-op assertions may be intentionally added as workarounds when ESLint rule jest/expect-expect doesn't detect expectations inside helper functions or waitFor callbacks. These can be resolved by configuring the ESLint rule's assertFunctionNames option to recognize custom assertion helpers instead of flagging them as redundant.
Applied to files:
frontend/__tests__/unit/components/ContributionStats.test.tsxfrontend/__tests__/unit/components/ContributionHeatmap.test.tsx
📚 Learning: 2025-07-03T03:08:03.290Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 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.
Applied to files:
frontend/__tests__/unit/components/ContributionHeatmap.test.tsx
🧬 Code graph analysis (1)
frontend/__tests__/unit/components/ContributionStats.test.tsx (1)
frontend/src/components/ContributionStats.tsx (1)
ContributionStats(22-82)
🔇 Additional comments (20)
frontend/__tests__/unit/components/ContributionStats.test.tsx (9)
4-18: LGTM: Clean mock implementation.The FontAwesome mocks are well-structured, allowing tests to verify icon presence via
data-iconattributes and supporting CSS class assertions through className passthrough.
38-71: LGTM: Thorough rendering verification.Tests properly verify title rendering, metric labels, basic number display, and locale-based formatting for larger numbers.
101-122: LGTM: Proper edge case coverage.Tests correctly verify that undefined, null, and empty stats objects all display '0' as fallback values, matching the component's
formatNumberbehavior.
124-166: LGTM: Comprehensive partial data handling.Tests properly verify that missing properties default to '0' while present values display correctly, including explicit zero values.
212-235: LGTM: Good loading state coverage.Tests properly verify that undefined stats display zeros during loading and that the component correctly updates when data becomes available.
237-265: LGTM: Solid accessibility verification.Tests properly verify heading hierarchy, semantic HTML structure, and screen reader-friendly labels for all metrics.
267-285: LGTM: Context-specific title verification.Tests appropriately verify the component works across different use cases (projects, chapters, board candidates) as outlined in the PR objectives.
287-307: LGTM: Good props handling verification.Tests properly verify readonly prop compatibility and dynamic title updates using rerender, ensuring the component responds correctly to prop changes.
309-343: LGTM: Comprehensive visual element verification.Tests properly verify CSS classes for styling and confirm all five FontAwesome icons render with correct data attributes, ensuring visual elements are present and identifiable.
frontend/__tests__/unit/components/ContributionHeatmap.test.tsx (11)
1-42: Well-structured mock setup.The mocking strategy is solid. Exposing chart configuration via data attributes enables thorough validation without relying on the actual ApexCharts library, and the next/dynamic mock correctly returns the mocked component.
44-72: LGTM: Proper test setup and cleanup.The beforeEach hook correctly resets mocks between tests, preventing state leakage. The default props and mock data provide a solid baseline for test scenarios.
74-123: LGTM: Comprehensive rendering validation.The tests appropriately use
waitForfor the dynamically-imported chart and verify essential rendering characteristics including chart type and series structure.
125-155: LGTM: Variant prop validation is thorough.Tests correctly verify that the variant prop controls chart dimensions and that the default variant is applied when unspecified.
157-204: LGTM: Theme handling is validated.The tests verify that the component handles light, dark, and undefined themes gracefully, with appropriate background settings.
206-250: Tests verify crash resistance but not edge-case behavior.These tests appropriately use
as anyto test invalid inputs and confirm the component doesn't crash. However, they don't verify how the component handles these edge cases (e.g., displaying a message, treating as empty, sanitizing values). This is acceptable for a "Chill" review level if the component is expected to be defensive.
252-310: LGTM: Partial data scenarios covered.Tests verify the component handles sparse data, single-day contributions, and out-of-range dates without crashing. For stricter validation, consider asserting whether out-of-range dates are filtered from the displayed data.
312-356: LGTM: Loading state transitions validated.The tests appropriately simulate loading states and verify smooth transitions from empty to populated data using
rerender.
435-480: LGTM: Unit prop variations tested.Tests verify the component accepts different unit values without issues. Since the mock doesn't expose tooltip content, validating unit string usage would require integration or E2E tests.
509-527: LGTM: Extreme value handling tested.The test verifies the component handles very high contribution counts without crashing. Actual rendering/formatting of these large numbers would be better validated in visual or integration tests.
565-585: LGTM: Variant-specific styling verified.Tests appropriately verify that variant-specific container classes are applied. Actual responsive behavior (viewport-based) would require visual regression or E2E tests.
frontend/__tests__/unit/components/ContributionHeatmap.test.tsx
Outdated
Show resolved
Hide resolved
frontend/__tests__/unit/components/ContributionHeatmap.test.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
backend/tests/apps/owasp/api/internal/nodes/project_test.py (1)
119-146: Consider integration tests for resolver behavior.The test correctly verifies the snake_case to camelCase transformation. However, accessing
field.base_resolver.wrapped_func(line 136) relies on Strawberry's internal API structure, which could break with library updates.For more robust testing, consider:
- GraphQL integration tests that execute actual queries and verify the response shape
- Separate unit tests for the
deep_camelizeutility if transformation logic needs isolated testingThe current approach works but couples tests to framework internals.
Example integration test approach
# Test via actual GraphQL query execution def test_contribution_stats_camel_case_via_query(self, project_with_stats): query = """ query { project(id: $id) { contributionStats } } """ result = schema.execute_sync(query, variable_values={"id": project_with_stats.id}) assert result.data["project"]["contributionStats"]["pullRequests"] == 50 assert "pull_requests" not in result.data["project"]["contributionStats"]frontend/src/components/ContributionHeatmap.tsx (1)
290-349: Consider adding ARIA attributes for better accessibility.The rendering structure is clean with proper responsive behavior and custom scrollbar styling. However, based on project learnings, ApexCharts doesn't accept ARIA attributes directly.
Consider wrapping the Chart component in a container with ARIA attributes to improve screen reader support:
<div className="inline-block"> + <div + role="img" + aria-label={`${title || 'Contribution heatmap'} showing contribution activity over ${heatmapSeries[0]?.data?.length || 0} weeks`} + tabIndex={0} + > <Chart options={options} series={heatmapSeries} type="heatmap" height={isCompact ? 150 : 195} width={chartWidth} /> + </div> </div>This provides context for assistive technology users without affecting visual presentation. Based on learnings from similar chart components in the codebase.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
frontend/src/types/__generated__/chapterQueries.generated.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/graphql.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/projectQueries.generated.tsis excluded by!**/__generated__/**
📒 Files selected for processing (11)
backend/apps/owasp/api/internal/nodes/chapter.pybackend/apps/owasp/api/internal/nodes/project.pybackend/tests/apps/owasp/api/internal/nodes/chapter_test.pybackend/tests/apps/owasp/api/internal/nodes/project_test.pyfrontend/__tests__/unit/components/CardDetailsPage.test.tsxfrontend/__tests__/unit/components/ContributionHeatmap.test.tsxfrontend/src/app/chapters/[chapterKey]/page.tsxfrontend/src/app/projects/[projectKey]/page.tsxfrontend/src/components/CardDetailsPage.tsxfrontend/src/components/ContributionHeatmap.tsxfrontend/src/types/card.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- backend/apps/owasp/api/internal/nodes/chapter.py
- frontend/src/app/projects/[projectKey]/page.tsx
- backend/tests/apps/owasp/api/internal/nodes/chapter_test.py
- frontend/src/app/chapters/[chapterKey]/page.tsx
🧰 Additional context used
🧠 Learnings (11)
📚 Learning: 2025-06-20T16:12:59.256Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 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.
Applied to files:
frontend/src/components/CardDetailsPage.tsxfrontend/src/types/card.tsfrontend/__tests__/unit/components/CardDetailsPage.test.tsx
📚 Learning: 2025-06-20T16:12:59.256Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 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.
Applied to files:
frontend/src/components/CardDetailsPage.tsxfrontend/src/types/card.tsfrontend/__tests__/unit/components/CardDetailsPage.test.tsx
📚 Learning: 2025-06-21T12:21:32.372Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 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.
Applied to files:
frontend/src/components/ContributionHeatmap.tsxfrontend/__tests__/unit/components/ContributionHeatmap.test.tsx
📚 Learning: 2025-04-30T13:41:20.846Z
Learnt from: codic-yeeshu
Repo: OWASP/Nest PR: 1444
File: frontend/src/components/NavDropDown.tsx:0-0
Timestamp: 2025-04-30T13:41:20.846Z
Learning: When implementing dropdown menus or similar interactive components, always include proper accessibility features: ARIA attributes (aria-expanded, aria-haspopup, aria-controls), keyboard navigation support (Enter, Space, Escape keys), and mechanisms to close dropdowns when clicking outside.
Applied to files:
frontend/src/components/ContributionHeatmap.tsx
📚 Learning: 2025-07-03T03:08:03.290Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 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.
Applied to files:
frontend/src/components/ContributionHeatmap.tsxfrontend/__tests__/unit/components/CardDetailsPage.test.tsxfrontend/__tests__/unit/components/ContributionHeatmap.test.tsx
📚 Learning: 2025-09-21T17:04:48.154Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2178
File: frontend/src/app/snapshots/[id]/page.tsx:0-0
Timestamp: 2025-09-21T17:04:48.154Z
Learning: User rudransh-shrivastava confirmed that suggested type safety improvements during Apollo Client migration were no longer relevant, reinforcing their preference to keep migration PRs focused on core migration changes rather than additional improvements.
Applied to files:
frontend/src/components/ContributionHeatmap.tsx
📚 Learning: 2025-07-12T17:36:57.255Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 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.
Applied to files:
frontend/__tests__/unit/components/CardDetailsPage.test.tsxfrontend/__tests__/unit/components/ContributionHeatmap.test.tsx
📚 Learning: 2025-07-11T15:57:56.648Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 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.
Applied to files:
backend/apps/owasp/api/internal/nodes/project.py
📚 Learning: 2025-12-31T05:17:39.659Z
Learnt from: kart-u
Repo: OWASP/Nest PR: 3101
File: backend/apps/common/extensions.py:92-98
Timestamp: 2025-12-31T05:17:39.659Z
Learning: In this codebase, import OperationType for GraphQL operations from the graphql-core package rather than from strawberry. Use 'from graphql import OperationType'. Strawberry re-exports via graphql-core internally, so relying on strawberry's API may be brittle. Apply this rule to all Python files that deal with GraphQL operation types; ensure imports come from graphql (graphql-core) and not from strawberry packages. This improves compatibility and avoids coupling to strawberry's internals.
Applied to files:
backend/apps/owasp/api/internal/nodes/project.pybackend/tests/apps/owasp/api/internal/nodes/project_test.py
📚 Learning: 2025-11-17T16:47:05.578Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:427-427
Timestamp: 2025-11-17T16:47:05.578Z
Learning: In the frontend test files for the OWASP/Nest repository, `expect(true).toBe(true)` no-op assertions may be intentionally added as workarounds when ESLint rule jest/expect-expect doesn't detect expectations inside helper functions or waitFor callbacks. These can be resolved by configuring the ESLint rule's assertFunctionNames option to recognize custom assertion helpers instead of flagging them as redundant.
Applied to files:
frontend/__tests__/unit/components/ContributionHeatmap.test.tsx
📚 Learning: 2025-11-17T17:30:42.139Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:169-171
Timestamp: 2025-11-17T17:30:42.139Z
Learning: In the OWASP/Nest frontend tests (PR #2671 context), wrapper functions like `expectChaptersCountEqualsThree` that simply call another helper with a fixed parameter (e.g., `expectChaptersCountEquals(3)`) are intentionally used to avoid arrow function callbacks in `waitFor` calls. This pattern prevents adding nesting depth that would trigger rule typescript:S2004. Example: `await waitFor(expectChaptersCountEqualsThree)` avoids the extra nesting from `await waitFor(() => expectChaptersCountEquals(3))`.
Applied to files:
frontend/__tests__/unit/components/ContributionHeatmap.test.tsx
🧬 Code graph analysis (2)
frontend/src/types/card.ts (2)
backend/apps/owasp/models/chapter.py (1)
Chapter(21-230)frontend/src/types/chapter.ts (1)
Chapter(4-29)
backend/apps/owasp/api/internal/nodes/project.py (2)
backend/apps/core/utils/index.py (1)
deep_camelize(54-71)backend/apps/owasp/api/internal/nodes/chapter.py (1)
contribution_stats(37-41)
🔇 Additional comments (17)
backend/tests/apps/owasp/api/internal/nodes/project_test.py (2)
19-20: LGTM! Field names correctly added to test validation.The addition of
contribution_dataandcontribution_statsto the expected field names ensures these new fields are validated by the test suite.
109-118: LGTM! Field type tests follow established patterns.Both tests correctly verify field presence and types. The distinction between
StrawberryOptional(for contribution_stats with nullable resolver) andNewType(for contribution_data auto-exposed from model) aligns with the implementation.backend/apps/owasp/api/internal/nodes/project.py (1)
6-6: LGTM! Implementation follows established patterns.The changes correctly implement the contribution data and stats exposure:
- Line 6: Import of
deep_camelizeutility supports the resolver transformation.- Line 27: Adding
contribution_datato the fields list enables Strawberry Django to auto-expose it from the model.- Lines 44-49: The
contribution_statsresolver mirrors the ChapterNode implementation pattern (per relevant code snippets), properly handling None values and transforming snake_case keys to camelCase for GraphQL clients.The implementation is well-typed, follows the established pattern from ChapterNode, and integrates cleanly with the existing codebase.
Also applies to: 27-27, 44-49
frontend/src/types/card.ts (1)
17-17: LGTM! Clean type extension for contribution features.The addition of
contributionData,contributionStats,startDate, andendDatetoDetailsCardPropsis well-structured. All new properties are optional, maintaining backward compatibility while enabling the new contribution heatmap functionality.Also applies to: 46-47, 51-51, 58-58
frontend/__tests__/unit/components/CardDetailsPage.test.tsx (2)
120-162: LGTM! Well-structured component mocks.The mocks for
ContributionHeatmapandContributionStatsproperly expose their props interfaces and render testable elements. The structured props validation ensures type safety during testing.
1552-1683: LGTM! Comprehensive test coverage for contribution features.The test suite thoroughly validates:
- Rendering of stats and heatmap when data is provided
- Type-specific title variants (Project vs Chapter)
- Conditional rendering based on data presence
- Heatmap gating on both data and date bounds
- Correct ordering relative to other sections
The tests are well-structured and cover both positive and negative cases.
frontend/src/components/CardDetailsPage.tsx (3)
19-20: LGTM! Clean component imports.The new imports for
ContributionHeatmapandContributionStatsfollow the established import pattern in this file.
45-48: LGTM! Props correctly integrated.The new contribution-related props are properly typed and optional, maintaining backward compatibility with existing usage.
248-274: LGTM! Well-implemented contribution section with proper guards.The contribution section implementation is solid:
- Conditional rendering based on data presence
- Type-specific title computation (Project/Chapter)
- Heatmap properly gated on both data and date bounds
- Good defensive check:
Object.keys(contributionData).length > 0- Responsive styling with appropriate container classes
The positioning between
entityLeadersandtopContributorsis logical.The implementation currently supports only "project" and "chapter" types for the title. If repositories should also display contribution activity, consider extending the title computation:
title={`${type === 'project' ? 'Project' : type === 'chapter' ? 'Chapter' : type === 'repository' ? 'Repository' : 'Contribution'} Contribution Activity`}Or use a helper function for clarity. Verify whether this feature should extend to other entity types based on the PR objectives.
frontend/__tests__/unit/components/ContributionHeatmap.test.tsx (3)
176-218: LGTM! Test updates align with component refactoring.The tests correctly reflect the new DOM structure:
- Container class updated from
.heatmap-containerto.w-full- Semantic heading level improved to
h3- Height expectation updated to explicit
195for default variant- Style element and tooltip presence validation maintained
Also applies to: 225-225
539-549: LGTM! Responsive test updates are appropriate.The tests correctly validate the new responsive container structure and style content presence.
621-676: LGTM! Comprehensive variant testing.The new test suite thoroughly validates:
- Default variant dimensions (195px height)
- Compact variant dimensions (150px height)
- Container styling for both variants (inline-block)
- Default behavior when variant not specified
- Consistent title styling across variants
The test coverage ensures the variant system works correctly.
frontend/src/components/ContributionHeatmap.tsx (5)
9-86: LGTM! Robust date handling with UTC consistency.The
generateHeatmapSeriesfunction implements solid defensive logic:
- Missing dates → 1-year default range
- Invalid dates → 1-year default range
- Invalid range (start > end) → swap dates
- UTC-based date operations throughout prevent timezone issues
- Week alignment from Monday
- Proper series reversal for display
The recursive fallback pattern is clean and ensures the function always returns valid data.
88-242: LGTM! Well-structured chart configuration with theme support.The
getChartOptionsfunction provides comprehensive chart configuration:
- Theme-aware color scales for activity levels (5 ranges)
- Clean minimal UI (no axes, labels, legend)
- Transparent background for theme flexibility
- Custom tooltip with proper date formatting (UTC-based)
- Proper stroke colors for dark/light modes
The configuration is well-organized and maintains visual consistency.
244-251: LGTM! Clean interface with variant support.The
ContributionHeatmapPropsinterface is well-defined:
- Required props clearly identified
- New
variantprop with type-safe literal union- Optional props provide flexibility
- Consistent with component usage patterns
253-288: LGTM! Efficient implementation with proper memoization.The component implementation demonstrates good practices:
- Proper
useMemousage for expensive calculations- Theme integration via
useThemehook- Sensible defaults (
unit='contribution',variant='default')- Dynamic width calculation based on week count
- Variant-specific scaling (compact: 13.4px/week, default: 19.5px/week)
- Minimum widths prevent UI collapse
180-211: Theunitparameter is safely used in current codebase, but consider documenting or validating this constraint.While the tooltip directly interpolates the
unitparameter into HTML (lines 197, 208) without escaping, all current usages pass hardcoded string literals:"contribution"(CardDetailsPage) and"message"(board candidates page). The parameter is not exposed to user input or external sources.To prevent future maintainers from accidentally passing unsafe values, add a defensive comment documenting this assumption, or consider a simple validation:
// In getChartOptions, add a comment: // unit is always internally controlled and safe to interpolate const unitLabel = count !== 1 ? `${unit}s` : unitThis is good defensive practice but not a current vulnerability.
mrkeshav-05
force-pushed
the
feature/contribution-heatmap-chapter-project
branch
from
15ecc9d to
db565f1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
frontend/src/app/chapters/[chapterKey]/page.tsx (1)
64-68: Use UTC date methods to prevent timezone skew.The date calculation mixes local-time operations (
setFullYear) with UTC serialization (toISOString), which can cause off-by-one day issues for users in negative UTC offsets. This is the same issue mentioned in previous reviews.🔎 Recommended fix using UTC methods
const today = new Date() +today.setUTCHours(0, 0, 0, 0) const oneYearAgo = new Date(today) -oneYearAgo.setFullYear(today.getFullYear() - 1) +oneYearAgo.setUTCFullYear(today.getUTCFullYear() - 1) const startDate = oneYearAgo.toISOString().split('T')[0] const endDate = today.toISOString().split('T')[0]frontend/src/components/ContributionHeatmap.tsx (1)
337-345: Add ARIA attributes for screen reader accessibility.The Chart component lacks accessibility attributes. Per project learnings, react-apexcharts does not accept ARIA props directly, so the Chart must be wrapped in a container with appropriate ARIA attributes to ensure screen reader users can understand the heatmap.
Based on learnings
🔎 Proposed fix
<div className="inline-block"> + <div + role="img" + aria-label={`${title || 'Contribution heatmap'} showing contribution activity over time`} + tabIndex={0} + > <Chart options={options} series={heatmapSeries} type="heatmap" height={isCompact ? 150 : 195} width={chartWidth} /> + </div> </div>
🧹 Nitpick comments (1)
frontend/src/components/ContributionHeatmap.tsx (1)
197-210: Consider validating or documenting theunitprop to prevent potential injection.While the
unitprop appears to be internally controlled (with a default of'contribution'), it's interpolated directly into the HTML tooltip string. To make the safety explicit and prevent future issues if the prop source changes, consider either:
- Adding a comment documenting that
unitmust be a trusted internal value, or- Validating
unitagainst a whitelist of allowed values🔎 Option 1: Add documentation comment
const isDarkMode = theme === 'dark' const isCompact = variant === 'compact' + // Note: unit prop must be a trusted internal value (e.g., 'contribution', 'commit') + // as it's interpolated into HTML. Never pass user-controlled input. const { heatmapSeries } = useMemo(🔎 Option 2: Add whitelist validation
const isDarkMode = theme === 'dark' const isCompact = variant === 'compact' + const allowedUnits = ['contribution', 'commit', 'issue', 'pull request', 'release'] + const safeUnit = allowedUnits.includes(unit) ? unit : 'contribution' const { heatmapSeries } = useMemo( () => generateHeatmapSeries(startDate, endDate, contributionData), [contributionData, startDate, endDate] ) - const options = useMemo(() => getChartOptions(isDarkMode, unit), [isDarkMode, unit]) + const options = useMemo(() => getChartOptions(isDarkMode, safeUnit), [isDarkMode, safeUnit])
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
frontend/src/types/__generated__/chapterQueries.generated.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/graphql.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/projectQueries.generated.tsis excluded by!**/__generated__/**
📒 Files selected for processing (12)
backend/apps/owasp/api/internal/nodes/chapter.pybackend/apps/owasp/api/internal/nodes/project.pybackend/tests/apps/owasp/api/internal/nodes/chapter_test.pybackend/tests/apps/owasp/api/internal/nodes/project_test.pyfrontend/__tests__/unit/components/CardDetailsPage.test.tsxfrontend/__tests__/unit/components/ContributionHeatmap.test.tsxfrontend/graphql-codegen.tsfrontend/src/app/chapters/[chapterKey]/page.tsxfrontend/src/app/projects/[projectKey]/page.tsxfrontend/src/components/CardDetailsPage.tsxfrontend/src/components/ContributionHeatmap.tsxfrontend/src/types/card.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/src/types/card.ts
- frontend/src/app/projects/[projectKey]/page.tsx
- backend/tests/apps/owasp/api/internal/nodes/chapter_test.py
🧰 Additional context used
🧠 Learnings (12)
📚 Learning: 2025-07-12T17:36:57.255Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 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.
Applied to files:
frontend/__tests__/unit/components/CardDetailsPage.test.tsxfrontend/__tests__/unit/components/ContributionHeatmap.test.tsx
📚 Learning: 2025-06-20T16:12:59.256Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 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.
Applied to files:
frontend/__tests__/unit/components/CardDetailsPage.test.tsxfrontend/src/components/CardDetailsPage.tsx
📚 Learning: 2025-06-20T16:12:59.256Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 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.
Applied to files:
frontend/__tests__/unit/components/CardDetailsPage.test.tsxfrontend/src/components/CardDetailsPage.tsx
📚 Learning: 2025-07-03T03:08:03.290Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 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.
Applied to files:
frontend/__tests__/unit/components/CardDetailsPage.test.tsxfrontend/src/components/ContributionHeatmap.tsxfrontend/__tests__/unit/components/ContributionHeatmap.test.tsx
📚 Learning: 2025-12-31T05:17:39.659Z
Learnt from: kart-u
Repo: OWASP/Nest PR: 3101
File: backend/apps/common/extensions.py:92-98
Timestamp: 2025-12-31T05:17:39.659Z
Learning: In this codebase, import OperationType for GraphQL operations from the graphql-core package rather than from strawberry. Use 'from graphql import OperationType'. Strawberry re-exports via graphql-core internally, so relying on strawberry's API may be brittle. Apply this rule to all Python files that deal with GraphQL operation types; ensure imports come from graphql (graphql-core) and not from strawberry packages. This improves compatibility and avoids coupling to strawberry's internals.
Applied to files:
backend/tests/apps/owasp/api/internal/nodes/project_test.pybackend/apps/owasp/api/internal/nodes/chapter.pybackend/apps/owasp/api/internal/nodes/project.py
📚 Learning: 2025-06-21T12:21:32.372Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 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.
Applied to files:
frontend/src/components/ContributionHeatmap.tsxfrontend/__tests__/unit/components/ContributionHeatmap.test.tsx
📚 Learning: 2025-04-30T13:41:20.846Z
Learnt from: codic-yeeshu
Repo: OWASP/Nest PR: 1444
File: frontend/src/components/NavDropDown.tsx:0-0
Timestamp: 2025-04-30T13:41:20.846Z
Learning: When implementing dropdown menus or similar interactive components, always include proper accessibility features: ARIA attributes (aria-expanded, aria-haspopup, aria-controls), keyboard navigation support (Enter, Space, Escape keys), and mechanisms to close dropdowns when clicking outside.
Applied to files:
frontend/src/components/ContributionHeatmap.tsx
📚 Learning: 2025-09-21T17:04:48.154Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2178
File: frontend/src/app/snapshots/[id]/page.tsx:0-0
Timestamp: 2025-09-21T17:04:48.154Z
Learning: User rudransh-shrivastava confirmed that suggested type safety improvements during Apollo Client migration were no longer relevant, reinforcing their preference to keep migration PRs focused on core migration changes rather than additional improvements.
Applied to files:
frontend/src/components/ContributionHeatmap.tsx
📚 Learning: 2025-11-17T16:47:05.578Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:427-427
Timestamp: 2025-11-17T16:47:05.578Z
Learning: In the frontend test files for the OWASP/Nest repository, `expect(true).toBe(true)` no-op assertions may be intentionally added as workarounds when ESLint rule jest/expect-expect doesn't detect expectations inside helper functions or waitFor callbacks. These can be resolved by configuring the ESLint rule's assertFunctionNames option to recognize custom assertion helpers instead of flagging them as redundant.
Applied to files:
frontend/__tests__/unit/components/ContributionHeatmap.test.tsx
📚 Learning: 2025-11-17T17:30:42.139Z
Learnt from: anurag2787
Repo: OWASP/Nest PR: 2671
File: frontend/__tests__/unit/components/MultiSearch.test.tsx:169-171
Timestamp: 2025-11-17T17:30:42.139Z
Learning: In the OWASP/Nest frontend tests (PR #2671 context), wrapper functions like `expectChaptersCountEqualsThree` that simply call another helper with a fixed parameter (e.g., `expectChaptersCountEquals(3)`) are intentionally used to avoid arrow function callbacks in `waitFor` calls. This pattern prevents adding nesting depth that would trigger rule typescript:S2004. Example: `await waitFor(expectChaptersCountEqualsThree)` avoids the extra nesting from `await waitFor(() => expectChaptersCountEquals(3))`.
Applied to files:
frontend/__tests__/unit/components/ContributionHeatmap.test.tsx
📚 Learning: 2025-07-11T15:57:56.648Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 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.
Applied to files:
backend/apps/owasp/api/internal/nodes/chapter.pybackend/apps/owasp/api/internal/nodes/project.py
📚 Learning: 2025-07-13T05:55:46.436Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 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.
Applied to files:
backend/apps/owasp/api/internal/nodes/chapter.py
🧬 Code graph analysis (4)
backend/tests/apps/owasp/api/internal/nodes/project_test.py (3)
backend/tests/apps/owasp/api/internal/nodes/chapter_test.py (4)
test_resolve_contribution_stats(60-63)_get_field_by_name(30-33)test_resolve_contribution_data(55-58)test_contribution_stats_transforms_snake_case_to_camel_case(65-92)backend/apps/owasp/api/internal/nodes/chapter.py (1)
contribution_stats(37-41)backend/apps/owasp/api/internal/nodes/project.py (1)
contribution_stats(45-49)
frontend/src/app/chapters/[chapterKey]/page.tsx (2)
frontend/src/utils/contributionDataUtils.ts (1)
getContributionStats(9-28)backend/apps/owasp/api/internal/queries/chapter.py (1)
chapter(14-19)
backend/apps/owasp/api/internal/nodes/chapter.py (2)
backend/apps/core/utils/index.py (1)
deep_camelize(54-71)backend/apps/owasp/api/internal/nodes/project.py (1)
contribution_stats(45-49)
backend/apps/owasp/api/internal/nodes/project.py (2)
backend/apps/core/utils/index.py (1)
deep_camelize(54-71)backend/apps/owasp/api/internal/nodes/chapter.py (1)
contribution_stats(37-41)
🔇 Additional comments (20)
backend/apps/owasp/api/internal/nodes/chapter.py (3)
6-6: LGTM: Import follows established pattern.The
deep_camelizeimport is correctly placed and matches the pattern used inbackend/apps/owasp/api/internal/nodes/project.py.
22-22: LGTM: Field addition is correct.The
contribution_datafield is properly exposed via the@strawberry_django.typedecorator's fields list, matching the pattern in ProjectNode.
36-41: LGTM: Resolver correctly transforms snake_case to camelCase.The resolver implementation is correct and matches the pattern in
backend/apps/owasp/api/internal/nodes/project.py:44-48. It properly handles None/empty cases and transforms the JSON keys usingdeep_camelize.frontend/__tests__/unit/components/ContributionHeatmap.test.tsx (4)
176-201: LGTM: Test expectations updated to match new rendering approach.The tests correctly verify the new container structure (
.w-fullinstead of.heatmap-container) and check for the presence of style content. This aligns with the component refactoring mentioned in the AI summary.
204-218: LGTM: Semantic HTML improvements.The heading level change from
h4toh3is an improvement in the heading hierarchy. The container class checks are appropriately updated to match the new implementation.
221-227: LGTM: Chart dimensions correctly updated.The height expectation changed from
'100%'to'195'matches the default variant height in the component implementation.
621-676: LGTM: Comprehensive variant test coverage.The new test suite thoroughly validates both default and compact variants, including:
- Correct dimensions (195px for default, 150px for compact)
- Container styling consistency
- Title styling across variants
- Default behavior when variant is unspecified
The height values align with the component implementation. Past review comments about the height mismatch appear to have been addressed.
frontend/src/app/chapters/[chapterKey]/page.tsx (1)
70-78: LGTM: Contribution data integration is correct.The
getContributionStatsutility is called correctly, and the new props (contributionData,contributionStats,startDate,endDate) are properly passed toDetailsCard. This aligns with the component's updated interface.Also applies to: 80-80, 86-86
frontend/graphql-codegen.ts (1)
59-60: LGTM: JSON scalar mapping correctly configured.The JSON scalar mapping to
Record<string, unknown>is the correct TypeScript representation for GraphQL's JSON scalar type. This is necessary for the newcontributionDataandcontributionStatsfields.backend/tests/apps/owasp/api/internal/nodes/project_test.py (3)
19-20: LGTM: Field names correctly added to test expectations.Both
contribution_dataandcontribution_statsare properly included in the expected field names set. This addresses the past review comment about the missing field.
109-112: LGTM: Field type test is correct.The test correctly validates that
contribution_statsis present and has typeStrawberryOptional, indicating it's nullable. This matches the pattern inchapter_test.py.
114-146: LGTM: Comprehensive field and transformation tests.The tests correctly validate:
contribution_datafield type asNewType(non-nullable)- The
contribution_statsresolver transforms snake_case keys to camelCase (pull_requests→pullRequests)- Original snake_case keys are not present in the result
This mirrors the pattern in
chapter_test.pyand provides thorough coverage.backend/apps/owasp/api/internal/nodes/project.py (3)
6-6: LGTM: Import follows established pattern.The
deep_camelizeimport matches the pattern inbackend/apps/owasp/api/internal/nodes/chapter.py:6.
27-27: LGTM: Field addition is correct.The
contribution_datafield is properly exposed via the@strawberry_django.typedecorator, matching the ChapterNode pattern.
44-49: LGTM: Resolver implementation is correct.The resolver correctly transforms snake_case keys to camelCase and properly handles None/empty cases. Implementation is identical to the ChapterNode resolver, ensuring consistency.
frontend/__tests__/unit/components/CardDetailsPage.test.tsx (2)
120-162: LGTM: Mock components well-designed for testing.The mock implementations for
ContributionHeatmapandContributionStatsare well-structured:
- Props are properly typed with TypeScript interfaces
- Mock output exposes testable content (day counts, stat values)
- Follows the pattern of other mocked components in this file
1552-1683: LGTM: Comprehensive test coverage for contribution features.The test suite thoroughly validates the contribution section functionality:
- Correct rendering when data is present/absent
- Appropriate title selection based on entity type (Project vs Chapter)
- Conditional rendering of stats and heatmap components
- Proper section ordering (contributions before top contributors)
- Edge cases (empty data, missing dates)
The tests use realistic data and appropriate assertions (e.g.,
compareDocumentPositionfor ordering).frontend/src/components/CardDetailsPage.tsx (3)
19-20: LGTM: Component imports correctly added.The imports for
ContributionHeatmapandContributionStatsare properly placed in alphabetical order and match the component usage below.
45-48: LGTM: Props correctly added to component signature.The new props (
contributionData,contributionStats,endDate,startDate) are properly destructured and align with theDetailsCardPropsinterface. They're used appropriately in the rendering logic.
248-274: LGTM: Contribution section rendering is well-implemented.The rendering logic is correct and robust:
- Conditionally renders when either
contributionDataorcontributionStatsis available- Title appropriately adapts based on entity type (Project vs Chapter)
ContributionStatsrenders when stats are presentContributionHeatmaponly renders when data is non-empty and date range is provided- Proper positioning between entity leaders and top contributors
- Styling is consistent with the rest of the component
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/src/components/CardDetailsPage.tsx (1)
85-95: LGTM!The pre-computed type checks effectively reduce cognitive complexity and improve code readability. All flags are properly utilized throughout the component.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/components/CardDetailsPage.tsx
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-06-20T16:12:59.256Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 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.
Applied to files:
frontend/src/components/CardDetailsPage.tsx
📚 Learning: 2025-06-20T16:12:59.256Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 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.
Applied to files:
frontend/src/components/CardDetailsPage.tsx
🧬 Code graph analysis (1)
frontend/src/components/CardDetailsPage.tsx (1)
backend/apps/owasp/api/internal/nodes/project.py (1)
repositories(109-116)
🔇 Additional comments (4)
frontend/src/components/CardDetailsPage.tsx (4)
19-20: LGTM!The new component imports are correctly added and maintain the existing import organization.
45-48: LGTM!The new props support the contribution heatmap and stats features. These props align with the DetailsCardProps interface updates mentioned in the PR.
167-167: LGTM!The refactored conditionals using the pre-computed type flags improve code readability and maintainability without changing behavior.
Also applies to: 203-203, 219-219, 316-316, 322-322
328-332: Verify that the repository section expansion is intentional.The repository section now renders for
'user'and'organization'types in addition to'project'. This appears to be a functional change rather than just a refactor.Confirm that:
- User and organization entities in your schema have a
repositoriesfield- This expansion aligns with the intended UX for user and organization detail pages
If confirmed, this change looks good and follows the same pattern as other type checks in the component.
mrkeshav-05
force-pushed
the
feature/contribution-heatmap-chapter-project
branch
from
58b39af to
db565f1
Compare
mrkeshav-05
force-pushed
the
feature/contribution-heatmap-chapter-project
branch
from
dc6fc4b to
f519930
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
frontend/src/components/ContributionHeatmap.tsx (2)
198-209: Add safety comment or whitelist forunitinterpolation.The
unitprop is interpolated into HTML at line 209. While currently trusted (defaulted to'contribution'), if this prop ever becomes user-derived, it's an injection vector. Consider adding a code comment noting it must remain trusted, or mapping through a whitelist.
338-346: Add ARIA wrapper for screen reader accessibility.The Chart component lacks accessibility attributes. Per project learnings, react-apexcharts doesn't support ARIA props directly—wrap the Chart in a container with appropriate attributes.
🔎 Proposed fix
<div className="inline-block"> + <div + role="img" + aria-label={`${title || 'Contribution heatmap'} showing activity over time`} + > <Chart options={options} series={heatmapSeries} type="heatmap" height={isCompact ? 150 : 195} width={chartWidth} /> + </div> </div>Based on learnings, react-apexcharts Chart component does not support ARIA attributes as direct props.
🧹 Nitpick comments (1)
frontend/src/components/ContributionHeatmap.tsx (1)
27-28: Ambiguous date parsing may cause timezone skew.
new Date("YYYY-MM-DD")parsing is implementation-dependent—some browsers interpret it as UTC midnight, others as local midnight. This conflicts with the UTC-consistent approach used elsewhere in the function (lines 56-83).🔎 Proposed fix to ensure UTC parsing
- const start = new Date(startDate) - const end = new Date(endDate) + const start = new Date(startDate + 'T00:00:00Z') + const end = new Date(endDate + 'T00:00:00Z')
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/components/CardDetailsPage.tsxfrontend/src/components/ContributionHeatmap.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/components/CardDetailsPage.tsx
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-06-21T12:21:32.372Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 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.
Applied to files:
frontend/src/components/ContributionHeatmap.tsx
📚 Learning: 2025-04-30T13:41:20.846Z
Learnt from: codic-yeeshu
Repo: OWASP/Nest PR: 1444
File: frontend/src/components/NavDropDown.tsx:0-0
Timestamp: 2025-04-30T13:41:20.846Z
Learning: When implementing dropdown menus or similar interactive components, always include proper accessibility features: ARIA attributes (aria-expanded, aria-haspopup, aria-controls), keyboard navigation support (Enter, Space, Escape keys), and mechanisms to close dropdowns when clicking outside.
Applied to files:
frontend/src/components/ContributionHeatmap.tsx
📚 Learning: 2025-07-03T03:08:03.290Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 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.
Applied to files:
frontend/src/components/ContributionHeatmap.tsx
📚 Learning: 2025-09-21T17:04:48.154Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2178
File: frontend/src/app/snapshots/[id]/page.tsx:0-0
Timestamp: 2025-09-21T17:04:48.154Z
Learning: User rudransh-shrivastava confirmed that suggested type safety improvements during Apollo Client migration were no longer relevant, reinforcing their preference to keep migration PRs focused on core migration changes rather than additional improvements.
Applied to files:
frontend/src/components/ContributionHeatmap.tsx
🔇 Additional comments (2)
frontend/src/components/ContributionHeatmap.tsx (2)
266-287: Good use of memoization for expensive computations.The
useMemohooks forheatmapSeries,options, andcalculateChartWidthappropriately prevent unnecessary recalculations. The variant-aware width calculation with minimum thresholds (400px compact, 600px default) is a sensible approach.
297-336: Scroll wrapper effectively handles overflow on smaller screens.The horizontal scroll container with hide-until-hover scrollbar behavior addresses the fixed-width overflow concern from previous reviews. The dynamic styling for dark/light mode scrollbars is a nice UX touch.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/__tests__/unit/components/CardDetailsPage.test.tsx (1)
1557-1688: Comprehensive test coverage for contribution features.The test suite effectively validates the rendering logic, conditional display, and DOM ordering for contribution stats and heatmap. The tests follow existing patterns and cover the key scenarios.
Optional: Two minor consistency improvements
1. Align topContributors structure with mockContributors (line 1668-1675):
topContributors: [ { - id: '1', login: 'user1', name: 'User One', avatarUrl: 'https://example.com/avatar1.png', + projectKey: 'test-project', + contributionsCount: 30, }, ],2. Add a test to verify other entity types don't render contribution sections:
it('does not render contribution section for non-project/chapter types', () => { const typesToTest = ['repository', 'user', 'organization', 'committee'] typesToTest.forEach(type => { cleanup() render( <CardDetailsPage {...defaultProps} type={type} contributionStats={contributionStats} contributionData={contributionData} /> ) expect(screen.queryByText(/Contribution Activity/i)).not.toBeInTheDocument() }) })
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/__tests__/unit/components/CardDetailsPage.test.tsxfrontend/src/app/board/[year]/candidates/page.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/app/board/[year]/candidates/page.tsx
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-07-12T17:36:57.255Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 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.
Applied to files:
frontend/__tests__/unit/components/CardDetailsPage.test.tsx
📚 Learning: 2025-06-20T16:12:59.256Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 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.
Applied to files:
frontend/__tests__/unit/components/CardDetailsPage.test.tsx
📚 Learning: 2025-06-20T16:12:59.256Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 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.
Applied to files:
frontend/__tests__/unit/components/CardDetailsPage.test.tsx
📚 Learning: 2025-07-03T03:08:03.290Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 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.
Applied to files:
frontend/__tests__/unit/components/CardDetailsPage.test.tsx
🔇 Additional comments (1)
frontend/__tests__/unit/components/CardDetailsPage.test.tsx (1)
120-162: LGTM! Well-structured mocks for contribution components.The mocks for
ContributionHeatmapandContributionStatsfollow the existing patterns in the file and provide appropriate test doubles with clear test IDs and minimal rendering logic.
mrkeshav-05
force-pushed
the
feature/contribution-heatmap-chapter-project
branch
from
82dbef7 to
b9e4db4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/components/CardDetailsPage.tsx
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-06-20T16:12:59.256Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 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.
Applied to files:
frontend/src/components/CardDetailsPage.tsx
📚 Learning: 2025-06-20T16:12:59.256Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 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.
Applied to files:
frontend/src/components/CardDetailsPage.tsx
🔇 Additional comments (5)
frontend/src/components/CardDetailsPage.tsx (5)
19-20: LGTM! Imports are correct.The new components are properly imported and will be used to render contribution data for projects and chapters.
58-61: LGTM! New props correctly integrated.The contribution data props and date range are properly added to support the new heatmap and statistics features.
168-168: LGTM! Consistent use of helper function.Replacing the inline conditional with
shouldShowStatistics(type)aligns with the refactoring and improves code clarity.
257-283: LGTM! Contribution block is well-guarded and correctly implemented.The type guard at line 257 ensures contributions are only rendered for projects and chapters (addressing the previous review feedback). All necessary data checks are in place before rendering the heatmap and stats, preventing runtime errors.
317-317: LGTM! Consistent use of helper functions.Both conditionals now use the centralized helper predicates, improving consistency and maintainability across the component.
Also applies to: 334-334
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/components/CardDetailsPage.tsx (1)
267-293: Consider guarding against empty container edge case.If
contributionDatais provided butstartDateorendDateare missing, andcontributionStatsdoesn't exist, the outer gray container will render without any content inside. While this scenario may be rare, you could refine the outer condition to prevent rendering an empty styled box.🔎 Optional refinement to prevent empty container
- {(type === 'project' || type === 'chapter') && (contributionData || contributionStats) && ( + {(type === 'project' || type === 'chapter') && + (contributionStats || + (contributionData && + Object.keys(contributionData).length > 0 && + startDate && + endDate)) && ( <div className="mb-8"> <div className="rounded-lg bg-gray-100 px-4 pt-6 shadow-md sm:px-6 lg:px-10 dark:bg-gray-800">This ensures the container only renders when there's actually content to display.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/__tests__/unit/components/CardDetailsPage.test.tsxfrontend/src/components/CardDetailsPage.tsxfrontend/src/types/card.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/tests/unit/components/CardDetailsPage.test.tsx
- frontend/src/types/card.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-06-20T16:12:59.256Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 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.
Applied to files:
frontend/src/components/CardDetailsPage.tsx
📚 Learning: 2025-06-20T16:12:59.256Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 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.
Applied to files:
frontend/src/components/CardDetailsPage.tsx
🔇 Additional comments (7)
frontend/src/components/CardDetailsPage.tsx (7)
19-20: LGTM! Clean imports.The new component imports follow the existing import pattern and integrate well with the rest of the file.
41-49: LGTM! Excellent type definition.The exported
CardTypeunion provides strong type safety and documents all supported entity types clearly.
51-62: LGTM! Well-designed helper functions.These exported predicates successfully centralize conditional rendering logic, making the code more maintainable and the type-based rules explicit.
178-178: LGTM! Clean refactor.Replacing the inline type checks with the
shouldShowStatisticshelper improves maintainability.
267-293: Well-implemented contribution UI block.The type guard
(type === 'project' || type === 'chapter')correctly restricts this feature to the intended entity types, and the conditional rendering properly handles both stats and heatmap display.
327-344: LGTM! Consistent refactoring.The use of helper predicates at both lines 327 and 344 maintains consistency with the refactoring approach and improves code clarity.
68-71: Type safety is already properly implemented.The new contribution-related props (
contributionData,contributionStats,endDate,startDate) are correctly typed in theDetailsCardPropsinterface withRecord<string, number>,ContributionStats,string, andstringrespectively. Thetypeprop is also properly typed asCardType(a union of valid card types) rather than a plain string, ensuring full type safety at compile time.
|
Hi @arkid15r , I've fixed the PR count and the heatmap scrollbar is hidden by default on standard screens (only appearing when width is too narrow to fit the heatmap content.). Screen.Recording.2026-01-03.at.12.14.47.AM.movI am currently looking at the 2 Sonar issues. One seems unrelated to my PR. Also, when I run |
|
3 participants
This PR extends the existing ContributionHeatmap component to Project and Chapter detail pages.
Fixed: #2625
Proposed change
ContributionStats.tsxto display the github stats.isCompactprop for different display contexts:contributionDataUtils.tsfor collecting the Github Statistics.Checklist
make check-testlocally; all checks and tests passed.