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 JSON fields for per-day contribution maps and per-type stats to Chapter/Project, a management command to aggregate contributions, GraphQL exposure, frontend types/utilities and UI components (heatmap + stats), Makefile targets, and tests covering backend aggregation and frontend components. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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
…efetch_related(coderabbitai)
mrkeshav-05
force-pushed
the
feature/contribution-heatmap-chapter-project
branch
from
d9bd9fa to
73df2ed
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
frontend/src/components/ContributionHeatmap.tsx (3)
181-212: Tooltip HTML interpolation assumesunitis trusted.The
unitprop is interpolated directly into HTML (line 209). While current usage passes hardcoded strings, ifunitever comes from user input, this could be an injection vector. Consider adding a comment documenting this assumption or using a whitelist:// unit is internal-only and must not contain user-controlled content const unitLabel = count !== 1 ? `${unit}s` : unit
14-24: Default-range fallback uses local time inconsistently with UTC logic elsewhere.The default date generation uses local-time methods (
setFullYear,getFullYear) but serializes withtoISOString(), which can cause off-by-one day issues for users in negative UTC offsets. The main loop (lines 56-83) correctly uses UTC methods.Apply UTC-consistent date handling:
if (!startDate || !endDate) { const defaultEnd = new Date() const defaultStart = new Date() - defaultStart.setFullYear(defaultEnd.getFullYear() - 1) + defaultEnd.setUTCHours(0, 0, 0, 0) + defaultStart.setUTCHours(0, 0, 0, 0) + defaultStart.setUTCFullYear(defaultEnd.getUTCFullYear() - 1) return generateHeatmapSeries( defaultStart.toISOString().split('T')[0], defaultEnd.toISOString().split('T')[0], contributionData ) }Apply the same fix to the invalid-date branch (lines 29-39).
301-311: Add ARIA attributes for screen reader accessibility.Per project learnings, the ApexCharts
Chartcomponent doesn't support ARIA attributes as direct props. Wrap the Chart in a container with appropriate accessibility attributes:- <div style={{ width: `${chartWidth}px`, maxWidth: '100%' }} className="min-w-fit"> - <div style={{ width: `${chartWidth}px` }}> + <div style={{ width: `${chartWidth}px`, maxWidth: '100%' }} className="min-w-fit"> + <div + style={{ width: `${chartWidth}px` }} + role="img" + aria-label={`${title || 'Contribution heatmap'} showing activity over time`} + > <Chart options={options} series={heatmapSeries} type="heatmap" height={isCompact ? 160 : 200} width="100%" /> </div> </div>Based on learnings about ApexCharts accessibility patterns.
🧹 Nitpick comments (4)
backend/apps/owasp/Makefile (1)
5-8: Consider aggregating both entity types in a single command invocation.The current implementation works correctly but makes two separate command invocations. If the management command supports aggregating both entity types in one call, you could simplify this to:
owasp-aggregate-contributions: @echo "Aggregating OWASP contributions" @CMD="python manage.py owasp_aggregate_contributions --entity-type chapter project" $(MAKE) exec-backend-commandHowever, if the command design requires separate invocations (e.g., for better error isolation or progress reporting), the current approach is fine.
backend/tests/apps/owasp/management/commands/owasp_aggregate_contributions_test.py (2)
28-30: MockQuerySet.filter() doesn't actually filter data.The
filter()method returnsselfwithout applying any filtering logic. While this works for basic chaining tests, it means tests liketest_handle_with_specific_key(line 359) cannot verify that the key filter is actually applied correctly to the data.Consider tracking filter kwargs if you need to verify filter behavior:
class MockQuerySet: """Mock QuerySet that supports slicing and iteration without database access.""" def __init__(self, items): self._items = items + self._filter_kwargs = [] def filter(self, **kwargs): - # Return self to support filter chaining + self._filter_kwargs.append(kwargs) return self
436-443: Timing tolerance is reasonable but consider increasing for CI stability.The 1-second tolerance at line 443 should work in most cases, but on heavily loaded CI runners, test execution between lines 434 and 440 could occasionally exceed this. Consider increasing to 2-3 seconds for more robust CI behavior, or mock
timezone.now()to eliminate timing sensitivity entirely.backend/apps/owasp/management/commands/owasp_aggregate_contributions.py (1)
249-261: Calling.count()on a potentially sliced queryset triggers an extra query.At line 251,
queryset.count()is called after the queryset may have been sliced (viaoffset). Django'scount()on a sliced queryset evaluates the slice first, then counts. Since you immediately iterate over the same queryset, consider usinglen(entities)after the list conversion for accurate counts without an extra query:def _process_entities(self, queryset, start_date, label, model_class): """Process entities (chapters or projects) for contribution aggregation.""" - count = queryset.count() - self.stdout.write(f"Processing {count} {label}...") - entities = [] for entity in queryset: entity.contribution_data = self.aggregate_contributions(entity, start_date) entity.contribution_stats = self.calculate_contribution_stats(entity, start_date) entities.append(entity) + count = len(entities) + self.stdout.write(f"Processing {count} {label}...") + if entities: model_class.bulk_save(entities, fields=("contribution_data", "contribution_stats")) self.stdout.write(self.style.SUCCESS(f"Updated {count} {label}"))
📜 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 (26)
backend/Makefile(1 hunks)backend/apps/owasp/Makefile(1 hunks)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/migrations/0067_chapter_contribution_stats_and_more.py(1 hunks)backend/apps/owasp/migrations/0068_alter_chapter_contribution_stats_and_more.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(2 hunks)backend/tests/apps/owasp/management/commands/owasp_aggregate_contributions_test.py(1 hunks)frontend/__tests__/unit/components/ContributionHeatmap.test.tsx(4 hunks)frontend/__tests__/unit/components/ContributionStats.test.tsx(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)frontend/src/utils/contributionDataUtils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- frontend/src/app/chapters/[chapterKey]/page.tsx
- frontend/src/utils/contributionDataUtils.ts
- backend/apps/owasp/models/project.py
- frontend/src/app/board/[year]/candidates/page.tsx
- backend/Makefile
- backend/apps/owasp/models/chapter.py
- frontend/src/server/queries/chapterQueries.ts
- backend/apps/owasp/migrations/0067_chapter_contribution_stats_and_more.py
- frontend/tests/unit/components/ContributionHeatmap.test.tsx
- frontend/src/types/project.ts
- backend/apps/owasp/api/internal/nodes/chapter.py
- frontend/src/types/chapter.ts
🧰 Additional context used
🧠 Learnings (13)
📚 Learning: 2025-11-23T11:37:26.253Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2606
File: backend/apps/api/rest/v0/project.py:43-48
Timestamp: 2025-11-23T11:37:26.253Z
Learning: In the OWASP Nest backend, `entity_leaders` is a `property` method defined in `RepositoryBasedEntityModel` (backend/apps/owasp/models/common.py) that returns a dynamically constructed QuerySet. It cannot be prefetched using standard `prefetch_related()` because Django's prefetch mechanism only works on model fields and relations, not property methods.
Applied to files:
backend/apps/owasp/management/commands/owasp_aggregate_contributions.py
📚 Learning: 2025-08-04T15:55:08.017Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1967
File: backend/apps/owasp/api/internal/filters/project_health_metrics.py:24-27
Timestamp: 2025-08-04T15:55:08.017Z
Learning: In the OWASP Nest project, the `is_active` filter in `ProjectHealthMetricsFilter` is intentionally designed as a workaround to eliminate inactive projects from the dashboard. It only filters FOR active projects when `value=True`, and returns an empty Q() when `value=False` to avoid showing inactive projects. This is not meant to be a general boolean filter but a specific solution to exclude inactive projects from the project health metrics dashboard.
Applied to files:
backend/apps/owasp/management/commands/owasp_aggregate_contributions.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
📚 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-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/ContributionStats.test.tsxfrontend/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/ContributionStats.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/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-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-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-08-31T13:47:15.861Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/src/server/queries/programsQueries.ts:81-81
Timestamp: 2025-08-31T13:47:15.861Z
Learning: In frontend/src/server/queries/programsQueries.ts, GET_PROGRAM_DETAILS and GET_PROGRAM_ADMIN_DETAILS are two separate queries serving different purposes: GET_PROGRAM_DETAILS fetches comprehensive program information while GET_PROGRAM_ADMIN_DETAILS fetches only admin-related details. These queries cannot be removed or merged as they serve different use cases in the application.
Applied to files:
frontend/src/server/queries/projectQueries.ts
📚 Learning: 2025-09-17T02:42:41.928Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 2288
File: frontend/src/components/ActionButton.tsx:0-0
Timestamp: 2025-09-17T02:42:41.928Z
Learning: In frontend/src/components/ActionButton.tsx, the user Rajgupta36 intentionally changed text-blue-600 to text-[#1D7BD7] to align the text color with the border color (#1D7BD7) for visual consistency, prioritizing design alignment over theme tokens.
Applied to files:
frontend/src/components/SponsorCard.tsx
🧬 Code graph analysis (6)
backend/apps/owasp/management/commands/owasp_aggregate_contributions.py (2)
backend/apps/owasp/models/chapter.py (2)
Chapter(21-230)bulk_save(194-205)backend/apps/owasp/models/project.py (2)
Project(37-397)bulk_save(364-372)
frontend/src/app/projects/[projectKey]/page.tsx (1)
frontend/src/utils/contributionDataUtils.ts (1)
getContributionStats(19-42)
backend/apps/owasp/migrations/0066_chapter_contribution_data_project_contribution_data.py (1)
backend/apps/owasp/migrations/0060_membersnapshot_chapter_project_contributions.py (1)
Migration(6-32)
backend/tests/apps/owasp/api/internal/nodes/project_test.py (1)
backend/tests/apps/owasp/api/internal/nodes/chapter_test.py (3)
test_resolve_contribution_stats(61-65)_get_field_by_name(30-33)test_resolve_contribution_data(55-59)
backend/tests/apps/owasp/api/internal/nodes/chapter_test.py (3)
backend/apps/owasp/api/internal/queries/chapter.py (1)
chapter(14-19)backend/tests/apps/owasp/api/internal/nodes/project_test.py (5)
test_meta_configuration(16-42)_get_field_by_name(44-47)test_resolve_key(64-67)test_resolve_contribution_data(114-118)test_resolve_contribution_stats(109-112)backend/apps/mentorship/api/internal/nodes/mentor.py (1)
name(18-20)
backend/tests/apps/owasp/management/commands/owasp_aggregate_contributions_test.py (1)
backend/apps/owasp/management/commands/owasp_aggregate_contributions.py (3)
_aggregate_contribution_dates(48-67)aggregate_contributions(82-140)handle(196-216)
🔇 Additional comments (17)
frontend/src/components/SponsorCard.tsx (1)
4-4: Verify that the margin removal is intentional and doesn't cause visual regressions.The
mb-8bottom margin utility was removed from the SponsorCard container. While this may be to adjust spacing for the new contribution components, please confirm:
- Whether this change is intentional and related to the contribution heatmap feature
- That pages using SponsorCard (Chapter/Project detail pages) maintain proper spacing after this adjustment
Consider testing the visual appearance on the affected pages to ensure the reduced spacing looks correct.
backend/apps/owasp/api/internal/nodes/project.py (1)
26-27: LGTM! GraphQL field additions are correct.The addition of
contribution_dataandcontribution_statsto ProjectNode follows the established pattern from ChapterNode. Strawberry Django will automatically expose these JSONField attributes as JSON scalars in the GraphQL schema, and test coverage has been addressed in previous commits.backend/tests/apps/owasp/api/internal/nodes/project_test.py (2)
19-20: LGTM! Test expectations updated correctly.The addition of
contribution_dataandcontribution_statstoexpected_field_namesensures the new GraphQL fields are validated bytest_meta_configuration.
109-118: LGTM! Test coverage is complete and follows established patterns.Both test methods correctly validate:
- Field presence in ProjectNode
- ScalarWrapper type (Strawberry's representation of JSONField)
- Consistent pattern with ChapterNode tests
frontend/src/app/projects/[projectKey]/page.tsx (2)
95-108: LGTM! Contribution data preparation is well-structured.The implementation correctly:
- Calculates a 1-year date range for the heatmap
- Uses
getContributionStatsutility to handle both new and legacy data formats- Separates concerns with
hasHeatmapDataandhasContributionStatsflags for independent rendering
131-156: LGTM! Conditional rendering handles partial data correctly.The rendering logic properly handles all scenarios:
- Shows the section when either stats or heatmap data exists
- Independently renders ContributionStats (when stats available) and ContributionHeatmap (when data available)
- Prevents showing empty sections when no data is present
This addresses the partial data handling requirement from the PR objectives.
frontend/__tests__/unit/components/ContributionStats.test.tsx (1)
1-348: LGTM! Exceptionally comprehensive test suite.This test file demonstrates excellent testing practices:
Strengths:
- Extensive coverage of rendering, formatting, and edge cases
- Proper handling of undefined, null, partial, and invalid data
- Accessibility validation (heading structure, semantic layout)
- Loading state transitions tested
- Type safety verification with readonly props
- All FontAwesome icon mocks correctly use
.dataset(addressed from previous review)Coverage includes:
- Normal rendering with formatted numbers (locale-aware)
- Edge cases: no data, partial data, zero values, negative values
- Extreme values (MAX_SAFE_INTEGER)
- Dynamic updates (title changes, data transitions)
- Visual structure validation
The test suite aligns with the learning that validation logic should be comprehensive at the component level.
backend/tests/apps/owasp/api/internal/nodes/chapter_test.py (2)
10-28: LGTM! Field expectations are complete and accurate.The
test_meta_configurationcorrectly validates all ChapterNode fields including the newcontribution_dataandcontribution_statsfields. The test structure mirrors ProjectNode tests for consistency.
55-65: LGTM! Contribution field tests follow established patterns.Both test methods correctly validate:
- Field presence using the
_get_field_by_namehelper- ScalarWrapper type assertion (Strawberry's representation of Django's JSONField)
- Consistent pattern with ProjectNode tests
The tests ensure GraphQL schema correctly exposes contribution data fields.
backend/apps/owasp/migrations/0068_alter_chapter_contribution_stats_and_more.py (1)
12-31: LGTM! Migration correctly aligns field definitions.The migration properly updates
contribution_statson both Chapter and Project models with:
blank=Trueanddefault=dictfor proper Django form handling- Descriptive
help_textdocumenting the field's structure- Clear
verbose_namefor admin interfaceThe identical field definitions across both models ensure consistency.
frontend/src/components/ContributionStats.tsx (3)
10-20: LGTM! Type definitions are clear and well-designed.The interfaces correctly:
- Use optional fields (
commits?,pullRequests?, etc.) to handle partial data- Mark props as
readonlyfor immutability- Separate data structure (
ContributionStatsData) from component props
22-25: LGTM! Number formatting handles edge cases correctly.The
formatNumberhelper properly:
- Checks for numeric type before formatting
- Uses
toLocaleString()for locale-aware formatting- Returns "0" as a safe default for undefined values
This approach is validated by the comprehensive test suite.
27-81: LGTM! Component rendering is well-structured and accessible.The implementation demonstrates good practices:
- Semantic HTML with proper heading structure (
<h2>)- Responsive grid layout (2 cols mobile, 4 cols desktop)
- Dark mode support via Tailwind classes
- FontAwesome icons for visual indicators
- Clear labeling for each metric
- Test ID for reliable testing
The component integrates well with the broader contribution display system alongside ContributionHeatmap.
frontend/src/server/queries/projectQueries.ts (1)
6-7: LGTM! New contribution fields added correctly.The
contributionDataandcontributionStatsfields are properly added to the GraphQL query and align with the backend schema changes inProjectNode.backend/apps/owasp/migrations/0066_chapter_contribution_data_project_contribution_data.py (1)
1-32: LGTM! Migration follows established patterns.The migration correctly adds
contribution_dataJSONField to both Chapter and Project models with appropriate defaults and help text. The structure aligns with the existing0060_membersnapshot_chapter_project_contributionsmigration pattern.backend/apps/owasp/management/commands/owasp_aggregate_contributions.py (1)
69-80: LGTM! Repository ID deduplication implemented correctly.The use of a
setforrepo_idsproperly handles the case whereowasp_repositorymight also be inrepositories, preventing double-counting. Thehasattrchecks gracefully handle differences between Chapter and Project entities.frontend/src/components/ContributionHeatmap.tsx (1)
266-278: Good use of memoization for expensive computations.The
useMemohooks forheatmapSeries,options, andchartWidthappropriately prevent unnecessary recalculations. The dependency arrays are correctly specified.
mrkeshav-05
force-pushed
the
feature/contribution-heatmap-chapter-project
branch
from
73df2ed to
e6d2fa3
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/src/components/ContributionHeatmap.tsx (1)
14-39: Timezone inconsistency in default date range generation.The fallback date generation (lines 14-24 and 29-38) uses local-time mutation (
setFullYear()) but serializes withtoISOString(), which can cause off-by-one-day shifts around DST boundaries or in negative UTC offsets. The main loop correctly uses UTC methods (lines 56-83), but the fallback doesn't match.Apply this diff to use UTC consistently in fallbacks:
if (!startDate || !endDate) { const defaultEnd = new Date() const defaultStart = new Date() - defaultStart.setFullYear(defaultEnd.getFullYear() - 1) + defaultEnd.setUTCHours(0, 0, 0, 0) + defaultStart.setUTCHours(0, 0, 0, 0) + defaultStart.setUTCFullYear(defaultEnd.getUTCFullYear() - 1) return generateHeatmapSeries( defaultStart.toISOString().split('T')[0], defaultEnd.toISOString().split('T')[0], contributionData ) }Apply the same pattern to the invalid-date fallback (lines 29-38).
🧹 Nitpick comments (2)
backend/apps/owasp/management/commands/owasp_aggregate_contributions.py (1)
69-80: Optional: Simplify list comprehension to generator.Line 78 uses a list comprehension inside
update(), butupdate()accepts any iterable. A generator expression would be slightly more efficient:# Handle multiple repositories (for projects) if hasattr(entity, "repositories"): - repo_ids.update([r.id for r in entity.repositories.all()]) + repo_ids.update(r.id for r in entity.repositories.all())This avoids creating an intermediate list.
frontend/src/components/ContributionHeatmap.tsx (1)
181-212: Clarify thatunitparameter is internal-only (XSS consideration).The tooltip HTML directly interpolates the
unitvariable (line 198). While current call sites pass hard-coded values ('contribution'), this could become an XSS vector ifunitbecomes user-controlled in the future.Add a comment clarifying the expectation:
custom: ({ seriesIndex, dataPointIndex, w }) => { const data = w.config.series[seriesIndex].data[dataPointIndex] if (!data) return '' const count = data.y const date = data.date const parsedDate = new Date(date + 'T00:00:00Z') const formattedDate = parsedDate.toLocaleDateString('en-US', { weekday: 'short', month: 'short', day: 'numeric', timeZone: 'UTC', }) const bgColor = isDarkMode ? '#1F2937' : '#FFFFFF' const textColor = isDarkMode ? '#F3F4F6' : '#111827' const secondaryColor = isDarkMode ? '#9CA3AF' : '#6B7280' + // unit is expected to be an internal, trusted string (e.g., 'contribution', 'commit') const unitLabel = count !== 1 ? `${unit}s` : unitAlternatively, use a whitelist:
+ const allowedUnits: Record<string, string> = { + contribution: 'contribution', + commit: 'commit', + issue: 'issue', + 'pull request': 'pull request', + } + const safeUnit = allowedUnits[unit] || 'contribution' - const unitLabel = count !== 1 ? `${unit}s` : unit + const unitLabel = count !== 1 ? `${safeUnit}s` : safeUnit
📜 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/management/commands/owasp_aggregate_contributions_test.py(1 hunks)frontend/__tests__/unit/components/ContributionHeatmap.test.tsx(4 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)
🧰 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/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/chapters/[chapterKey]/page.tsxfrontend/src/app/projects/[projectKey]/page.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.tsxfrontend/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-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/__tests__/unit/components/ContributionHeatmap.test.tsxfrontend/src/components/ContributionHeatmap.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-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/ContributionHeatmap.test.tsx
📚 Learning: 2025-11-23T11:37:26.253Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2606
File: backend/apps/api/rest/v0/project.py:43-48
Timestamp: 2025-11-23T11:37:26.253Z
Learning: In the OWASP Nest backend, `entity_leaders` is a `property` method defined in `RepositoryBasedEntityModel` (backend/apps/owasp/models/common.py) that returns a dynamically constructed QuerySet. It cannot be prefetched using standard `prefetch_related()` because Django's prefetch mechanism only works on model fields and relations, not property methods.
Applied to files:
backend/apps/owasp/management/commands/owasp_aggregate_contributions.py
📚 Learning: 2025-08-04T15:55:08.017Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1967
File: backend/apps/owasp/api/internal/filters/project_health_metrics.py:24-27
Timestamp: 2025-08-04T15:55:08.017Z
Learning: In the OWASP Nest project, the `is_active` filter in `ProjectHealthMetricsFilter` is intentionally designed as a workaround to eliminate inactive projects from the dashboard. It only filters FOR active projects when `value=True`, and returns an empty Q() when `value=False` to avoid showing inactive projects. This is not meant to be a general boolean filter but a specific solution to exclude inactive projects from the project health metrics dashboard.
Applied to files:
backend/apps/owasp/management/commands/owasp_aggregate_contributions.py
📚 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
🧬 Code graph analysis (4)
frontend/src/app/chapters/[chapterKey]/page.tsx (2)
frontend/src/utils/contributionDataUtils.ts (1)
getContributionStats(19-42)backend/apps/owasp/api/internal/queries/chapter.py (1)
chapter(14-19)
frontend/src/app/projects/[projectKey]/page.tsx (1)
frontend/src/utils/contributionDataUtils.ts (1)
getContributionStats(19-42)
backend/tests/apps/owasp/management/commands/owasp_aggregate_contributions_test.py (1)
backend/apps/owasp/management/commands/owasp_aggregate_contributions.py (4)
Command(16-262)_aggregate_contribution_dates(48-67)aggregate_contributions(82-140)handle(196-216)
backend/apps/owasp/management/commands/owasp_aggregate_contributions.py (3)
backend/apps/github/models/commit.py (1)
Commit(11-110)backend/apps/owasp/models/chapter.py (2)
Chapter(21-230)bulk_save(194-205)backend/apps/owasp/models/project.py (2)
Project(37-397)bulk_save(364-372)
🔇 Additional comments (16)
frontend/src/app/projects/[projectKey]/page.tsx (2)
95-110: LGTM: Date range and stats calculation is well-structured.The 1-year lookback calculation and conditional stats retrieval are correctly implemented. The
getContributionStatsutility provides backward compatibility with legacy data, and the boolean guards (hasHeatmapData,hasContributionStats) enable independent rendering of heatmap and stats components.
132-157: LGTM: Partial data handling correctly implemented.The conditional rendering properly addresses the previous review feedback by:
- Showing the contribution section when either
hasContributionStatsORhasHeatmapDatais true- Independently rendering
ContributionStatsandContributionHeatmapbased on data availability- The non-null assertion at line 146 is safe due to the
hasHeatmapDataguardThis ensures the UI gracefully handles partial data scenarios (e.g., stats available but no heatmap data, or vice versa).
frontend/src/app/chapters/[chapterKey]/page.tsx (2)
66-81: LGTM: Consistent implementation with project page.The date range calculation and contribution stats retrieval follow the same pattern as the project page, ensuring consistency across chapter and project detail pages.
96-121: LGTM: Proper partial data handling.The conditional rendering correctly implements the same partial-data pattern as the project page, with independent guards for stats and heatmap components.
frontend/__tests__/unit/components/ContributionHeatmap.test.tsx (2)
622-677: LGTM: Comprehensive variant test coverage.The new variant tests properly verify:
- Default variant renders with 200px height
- Compact variant renders with 160px height
- Container styling applies correctly for both variants
- Title styling remains consistent regardless of variant
The ThemeProvider wrapping in rerenders is now consistent (addressing previous feedback).
176-226: LGTM: Test assertions updated for new component structure.The updated expectations correctly reflect the component's refactored structure:
w-fullclass for responsive containerh3element for semantic title headingdata-heightattribute for variant-specific height verificationapexcharts-tooltippresence in style contentAll assertions align with the actual component implementation.
backend/tests/apps/owasp/management/commands/owasp_aggregate_contributions_test.py (4)
12-51: LGTM: MockQuerySet implementation is adequate for test needs.The
MockQuerySetproperly simulates Django ORM behavior for testing:
- Supports slicing for offset testing
- Chains filter, order_by, select_related, and prefetch_related
- Provides count() and len() for assertions
This enables effective testing of the management command without database dependencies.
97-122: LGTM: Helper method test provides solid coverage.The test for
_aggregate_contribution_datesproperly verifies:
- Multiple contributions on the same day aggregate to a single count (2 on 2024-11-16)
- Different days are tracked separately
- None values are correctly skipped
127-197: LGTM: Aggregation tests cover key scenarios.The aggregation tests properly verify:
- Chapter contributions aggregated from single
owasp_repository- Project contributions aggregated across multiple repositories
- All contribution types (commits, issues, PRs, releases) included
- Correct per-date grouping and counting
218-443: LGTM: Command execution tests provide comprehensive coverage.The command execution tests properly verify:
- Entity type filtering (chapters, projects, both)
- Key-based filtering for specific entities
- Offset parameter correctly skips entities (with assertion confirming only 1 processed after offset of 2)
- Custom days parameter affects start_date calculation (with 1-second tolerance)
bulk_saveinvoked with correct fieldsAll major command options and flows are covered. The offset test now includes the assertion that was missing in previous review feedback.
backend/apps/owasp/management/commands/owasp_aggregate_contributions.py (4)
82-140: LGTM: Contribution aggregation logic is correct.The
aggregate_contributionsmethod properly:
- Returns early when no repositories exist
- Uses the refactored
_aggregate_contribution_dateshelper for each contribution type- Applies correct date fields (
created_atfor commits/issues/PRs,published_atfor releases)- Excludes draft releases from aggregation
142-194: LGTM: Stats calculation uses correct camelCase keys.The
calculate_contribution_statsmethod properly addresses previous feedback:
- Uses camelCase keys (
pullRequestsinstead ofpull_requests) for frontend compatibility- Correctly counts each contribution type
- Applies appropriate filters (including
is_draft=Falsefor releases)- Calculates total as sum of all types
218-247: LGTM: Query optimization and ordering correctly implemented.Both
_process_chaptersand_process_projectsproperly address previous feedback:
- Explicit
order_by("id")ensures deterministic paginationselect_related("owasp_repository")prevents N+1 queries for chaptersselect_related+prefetch_related("repositories")optimizes project queries- Query optimizations applied before offset slicing for correct DB query generation
249-262: LGTM: Entity processing is efficient and correct.The
_process_entitiesmethod:
- Calls
count()on the optimized queryset (before any slicing in parent methods)- Iterates once, building the entities list
- Sets both
contribution_dataandcontribution_statsfields- Uses
bulk_savewith explicit fields for efficient updatesfrontend/src/components/ContributionHeatmap.tsx (2)
245-279: LGTM: Variant implementation is clean and well-memoized.The
variantprop correctly controls:
- Pixels per week (16 for compact, 20 for default)
- Minimum width of 600px with dynamic calculation
- Chart height (line 307: 160 for compact, 200 for default)
Proper use of
useMemoprevents unnecessary recalculations.
280-315: Verify accessibility: ARIA attributes not present.The rendering structure properly implements:
- Horizontal scroll for small screens (addressing feedback about GitHub-like behavior)
- Semantic
h3for title- Responsive width with
w-fulland overflow handlingHowever, a previous review suggested adding ARIA attributes (role="img", aria-label) to improve screen reader accessibility. The past comment is marked as addressed, but the current code doesn't include these attributes.
Was the ARIA implementation intentionally omitted or reverted? If accessibility attributes are still desired, consider wrapping the Chart:
<div role="img" aria-label={`Contribution heatmap${title ? `: ${title}` : ''} showing activity over time`} > <Chart options={options} series={heatmapSeries} type="heatmap" height={isCompact ? 160 : 200} width="100%" /> </div>Based on learnings, react-apexcharts doesn't support ARIA props directly, so wrapping is the recommended approach.
|
Hi @Arkid , I have updated the changes. Can you review it once. |
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.