feat: implement multi-column sorting for project health metrics #2493
Conversation
Resolves OWASP#2002 - Add sortable fields: stars, forks, contributors, createdAt to backend - Implement 3-state sorting cycle (desc → asc → unsorted) - Add URL parameter handling with format: -fieldName (desc) or fieldName (asc) - Default sorting: -score (descending) - Fix URL/state desync for browser back/forward navigation - Add type-safe FIELD_MAPPING with 'as const' - Improve accessibility with ARIA attributes - Add comprehensive test coverage for sorting behavior - Update GraphQL types with new sortable fields Backend tests and frontend tests passing.
Summary by CodeRabbit
WalkthroughAdds four new orderable fields (stars_count, forks_count, contributors_count, created_at) to backend ProjectHealthMetricsOrder, updates backend tests, and implements multi-field, URL-driven sortable column headers with tie-breaker support and tests in the frontend metrics dashboard. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (5 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 |
|
The linked issue must be assigned to the PR author. |
|
Hi @arkid15r , |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsx (1)
151-196: Strengthen URL parameter assertions and consider UI state verification.The test correctly validates the sorting state transitions through URL updates, and the local mock setup provides good test isolation. However, the string-based assertions using
.toContain()are fragile and could match partial strings or unintended parameters.Recommendation 1: Parse URLs for robust parameter validation
Replace string matching with URL parsing:
await waitFor(() => { - expect(mockReplace).toHaveBeenCalledWith(expect.stringContaining('order=-stars')) + const lastCall = mockReplace.mock.calls[mockReplace.mock.calls.length - 1][0] + const url = new URL(lastCall, 'http://localhost') + expect(url.searchParams.get('order')).toBe('-stars') })Apply similar changes to lines 179-182 and 193-195 for more precise assertions.
Recommendation 2 (optional): Verify UI state reflects sorting
Consider adding assertions to verify that the sort button displays the correct icon or active state after each click:
// After clicking, verify the UI shows descending state expect(sortButton).toHaveAttribute('data-sort-state', 'desc') // or similarNote on line 184 comment: The comment states "defaults to -score" but the test only verifies that the order parameter is removed. Consider clarifying that the default is applied on the backend when the parameter is absent, if that's the intended behavior.
To confirm the current UI state rendering works correctly with the sorting behavior, you may want to verify that the SortableColumnHeader component updates its visual state appropriately in the actual implementation.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsx(3 hunks)
🔇 Additional comments (1)
frontend/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsx (1)
117-149: LGTM! Test correctly validates sortable column headers.The test appropriately verifies that sortable column headers are rendered for each sortable field (Stars, Forks, Contributors, Health Checked At, Score) by checking for sort buttons with the expected title attributes.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsx (1)
151-199: Solid coverage of the sorting state cycle.The test comprehensively validates the 3-state sorting transitions (unsorted → descending → ascending → unsorted) and correctly verifies URL parameter updates through mocked router calls.
Consider these optional enhancements:
Verify pagination reset: The PR objectives mention "Pagination resets to page 1 when sort changes." Consider asserting that the
pageparameter is reset to 1 (or removed) in the URL updates.Test additional columns: Currently only the Stars column is tested through all transitions. Consider adding a test case for at least one other column (e.g., Score or Forks) to ensure the pattern works consistently across different field mappings.
Example addition for pagination verification:
await waitFor(() => { const lastCall = mockReplace.mock.calls[mockReplace.mock.calls.length - 1][0] const url = new URL(lastCall, 'http://localhost') expect(url.searchParams.get('order')).toBe('-stars') + expect(url.searchParams.get('page')).toBeNull() // or toBe('1') })
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsx(3 hunks)
🔇 Additional comments (1)
frontend/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsx (1)
117-149: Test updates align well with the new multi-column sorting UI.The renamed test accurately reflects the shift from dropdown-based sorting to sortable column headers. The verification of sortable column headers via title attributes (
Sort by ${column}) is a solid approach that ensures all five sortable columns are properly rendered.
|
On CodeRabbit nitpick:
|
|
Hi @arkid15r , Pr is ready for review. Thanks! |
| project_Name: Ordering.Asc, | ||
| }, | ||
| ] | ||
| const SortableColumnHeader: FC<{ |
There was a problem hiding this comment.
Should be a separate component?
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
frontend/src/app/projects/dashboard/metrics/page.tsx (2)
59-102: LGTM! Well-implemented sortable column header.The SortableColumnHeader component correctly implements the 3-state sorting cycle (none → descending → ascending → none) with appropriate visual indicators and accessibility attributes. The component is now separated from the main component logic as suggested in the previous review.
Note: While the component is now defined separately, it remains in the same file. If you anticipate reusing this component elsewhere, consider extracting it to
frontend/src/components/SortableColumnHeader.tsx.
181-188: Consider more reliable object comparison.The useEffect correctly syncs ordering from URL changes, but the JSON.stringify comparison (line 184) is fragile—key order differences could cause false inequality. While this works for simple single-field objects, a more reliable approach would be to compare the field and direction properties directly:
- if (JSON.stringify(nextOrdering) !== JSON.stringify(ordering)) { + const currentKey = Object.keys(ordering)[0] + const nextKey = Object.keys(nextOrdering)[0] + if (currentKey !== nextKey || ordering[currentKey] !== nextOrdering[nextKey]) { setOrdering(nextOrdering) }Alternatively, use a deep equality utility like Lodash's
isEqualfor more robust comparison.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/apps/owasp/api/internal/ordering/project_health_metrics.py(1 hunks)backend/tests/apps/owasp/api/internal/ordering/project_health_metrics_test.py(1 hunks)frontend/src/app/projects/dashboard/metrics/page.tsx(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/tests/apps/owasp/api/internal/ordering/project_health_metrics_test.py
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-07-09T08:37:10.241Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1718
File: frontend/src/app/projects/dashboard/metrics/page.tsx:96-96
Timestamp: 2025-07-09T08:37:10.241Z
Learning: In the OWASP Nest project's MetricsPage component (frontend/src/app/projects/dashboard/metrics/page.tsx), the sorting dropdown intentionally uses selectionMode="multiple" to allow users to select multiple sorting criteria simultaneously. This enables complex sorting scenarios where users can sort by multiple fields in sequence.
Applied to files:
frontend/src/app/projects/dashboard/metrics/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/dashboard/metrics/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/projects/dashboard/metrics/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/dashboard/metrics/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/dashboard/metrics/page.tsx
🧬 Code graph analysis (1)
backend/apps/owasp/api/internal/ordering/project_health_metrics.py (2)
backend/apps/owasp/api/internal/nodes/committee.py (3)
contributors_count(15-17)forks_count(25-27)stars_count(40-42)backend/apps/owasp/api/internal/nodes/project_health_metrics.py (1)
created_at(45-47)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Run backend tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
🔇 Additional comments (8)
backend/apps/owasp/api/internal/ordering/project_health_metrics.py (1)
13-24: LGTM! Ordering fields correctly expanded.The addition of contributors_count, created_at, forks_count, and stars_count as orderable fields is correct and aligns with the resolvers in the GraphQL nodes. The tie-breaker comment on line 17 appropriately documents the use of project__name for deterministic pagination.
frontend/src/app/projects/dashboard/metrics/page.tsx (7)
3-4: LGTM! Icon imports correctly added.The FontAwesome sorting icons (faSort, faSortUp, faSortDown) and FontAwesomeIcon component are properly imported for the sortable column headers UI.
20-28: LGTM! Type-safe field mapping.The FIELD_MAPPING constant correctly maps frontend field keys to camelCase GraphQL field names, which aligns with strawberry-django's automatic snake_case to camelCase conversion. The derived OrderKey type ensures type safety across sorting operations.
30-44: LGTM! Robust order parameter parsing.The parseOrderParam function correctly handles URL order parameters with proper validation, fallback to default ordering (score descending), and type-safe field mapping. The logic appropriately handles the '-' prefix for descending order.
141-143: LGTM! Correct ordering initialization and query integration.The ordering is properly initialized from URL parameters, stored in state, and passed to the GraphQL query with the tie-breaker. The flow ensures consistency between URL state and query variables.
Also applies to: 166-166, 177-177
228-244: LGTM! Correct sort handling with pagination reset.The handleSort function properly resets pagination when sorting changes (line 229), updates URL parameters, and maintains state consistency. Using router.replace is appropriate for sort changes as they shouldn't create separate history entries.
288-324: LGTM! Consistent sortable column headers.The column headers correctly use the SortableColumnHeader component with appropriate field keys, current state, and sort handlers. The grid layout accommodates all columns, and the center alignment for data columns is appropriate.
352-352: LGTM! Consistent ordering in pagination.The fetchMore call correctly applies the same ordering with tie-breaker as the initial query, ensuring consistent results across paginated data.
| const buildOrderingWithTieBreaker = (primaryOrdering: Record<string, Ordering>) => [ | ||
| primaryOrdering, | ||
| { | ||
| // eslint-disable-next-line @typescript-eslint/naming-convention | ||
| project_Name: Ordering.Asc, | ||
| }, | ||
| ] |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the tie-breaker field name matches the GraphQL schema.
The tie-breaker uses project_Name (line 56) with an underscore and capital N, which is inconsistent with the camelCase convention used elsewhere (contributorsCount, forksCount, etc.). The backend defines project__name (double underscore), which strawberry-django typically converts to projectName in the GraphQL schema.
If the field name doesn't match the actual GraphQL schema, the tie-breaker will be ignored, potentially causing inconsistent pagination results.
Run the following script to verify the exact field name in the GraphQL schema:
🏁 Script executed:
#!/bin/bash
# Description: Check the GraphQL schema for ProjectHealthMetricsOrder field names
# Search for the ProjectHealthMetricsOrder type definition in schema or generated types
rg -n "ProjectHealthMetricsOrder" --type=ts --type=graphql -A 20
# Also check for any project-related ordering fields
rg -n "project.*[Nn]ame.*Order" --type=ts --type=graphql -C 3Length of output: 63
🏁 Script executed:
#!/bin/bash
# Find GraphQL schema files with common extensions
echo "=== Looking for GraphQL schema files ==="
fd -e graphql -e gql -e schema.ts -e types.ts | head -20
# Search for ProjectHealthMetricsOrder in all files
echo -e "\n=== Searching for ProjectHealthMetricsOrder ==="
rg -n "ProjectHealthMetricsOrder" -A 10
# Search for project_Name or projectName in ordering context
echo -e "\n=== Searching for project_Name or projectName in code ==="
rg -n "project_[Nn]ame|projectName" --type=ts -B 2 -A 2
# Check the actual metrics page file
echo -e "\n=== Checking the metrics page file ==="
rg -n "buildOrderingWithTieBreaker\|Ordering\." frontend/src/app/projects/dashboard/metrics/page.tsx -B 3 -A 3Length of output: 74097
Fix the tie-breaker field name from project_Name to projectName.
The GraphQL schema defines the ordering field as project_Name, but this violates the camelCase convention used throughout the schema. The actual result field is projectName, and all other ordering fields follow camelCase naming (contributorsCount, createdAt, forksCount). The mismatch means the tie-breaker will fail silently during pagination.
Change line 56 from project_Name to projectName.
🤖 Prompt for AI Agents
In frontend/src/app/projects/dashboard/metrics/page.tsx around lines 52 to 58,
the tie-breaker ordering uses the wrong field name `project_Name`; change it to
the camelCase field `projectName` so the ordering matches actual result fields
(e.g., contributorsCount, createdAt) and prevents silent pagination failures —
update the object key on line 56 from project_Name to projectName.
2 participants
Proposed change
Resolves #2002
Implemented multi-column sorting functionality for the Project Health Metrics Dashboard, allowing users to sort by any visible column (Stars, Forks, Contributors, Health Check Date, Score) with ascending/descending order controlled via clickable column headers.
Changes:
Backend:
ProjectHealthMetricsOrder:stars_count,forks_count,contributors_count,created_atscoreandproject__name)Frontend:
SortableColumnHeadercomponent with visual feedback (sort icons and active state highlighting)-fieldName(descending) orfieldName(ascending)-score(score descending)parseOrderParam(),buildGraphQLOrdering(),buildOrderingWithTieBreaker()Files modified: 4 files
backend/apps/owasp/api/internal/ordering/project_health_metrics.pybackend/tests/apps/owasp/api/internal/ordering/project_health_metrics_test.pyfrontend/src/app/projects/dashboard/metrics/page.tsxfrontend/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsxFeatures:
?order=-stars,?order=forks, etc.Checklist
make check-testlocally; all checks and tests passed.##Images
