feat: add multi-column sorting to project health metrics dashboard #2492
Conversation
- Add sortable fields: stars, forks, contributors, health checked date - Replace dropdown UI with clickable column headers - Implement 3-state sorting: unsorted → desc → asc → unsorted - URL format: -forks, stars, -contributors, etc. - Visual feedback with sort icons (▼ ▲ ⬍) - Default sorting: -score (score descending) - Backend: expose new ordering fields in GraphQL schema - Tests: update to verify all new ordering fields
|
Caution Review failedThe pull request is closed. Summary by CodeRabbit
WalkthroughAdds backend ordering fields (stars_count, forks_count, contributors_count, created_at), updates backend tests for multi-field ordering, and implements frontend sortable column headers with URL-synced multi-field ordering and tie-breaker behavior; updates corresponding frontend tests and CI workflow messages. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (4)
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 |
|
Test: The linked issue must be assigned to the PR author. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
frontend/src/app/projects/dashboard/metrics/page.tsx (3)
28-39: Canonicalize invalidorderinputs to default and reflect it back in UI.If
?order=contains an unknown key, you fall back toscorefor GraphQL but keep the raw URL key for UI, causing icon mismatch. Canonicalize the URL key too.Apply:
const parseOrderParam = (orderParam: string | null) => { if (!orderParam) { return { field: 'score', direction: Ordering.Desc, urlKey: '-score' } } const isDescending = orderParam.startsWith('-') const fieldKey = isDescending ? orderParam.slice(1) : orderParam - const graphqlField = FIELD_MAPPING[fieldKey] || 'score' - const direction = isDescending ? Ordering.Desc : Ordering.Asc - - return { field: graphqlField, direction, urlKey: orderParam } + const isValidKey = fieldKey in FIELD_MAPPING + const normalizedKey = isValidKey ? fieldKey : 'score' + const graphqlField = FIELD_MAPPING[normalizedKey] + const direction = isDescending ? Ordering.Desc : Ordering.Asc + const normalizedUrlKey = direction === Ordering.Desc ? `-${normalizedKey}` : normalizedKey + return { field: graphqlField, direction, urlKey: normalizedUrlKey } }
20-26: Tighten types forFIELD_MAPPINGto prevent drift.Use
as constto derive a safeOrderKeyunion and avoid silent typos across UI ↔ GraphQL fields.Apply:
-const FIELD_MAPPING: Record<string, string> = { +const FIELD_MAPPING = { score: 'score', stars: 'starsCount', forks: 'forksCount', contributors: 'contributorsCount', createdAt: 'createdAt', -} +} as const +type OrderKey = keyof typeof FIELD_MAPPINGThen update
SortableColumnHeaderprop and usages tofieldKey: OrderKey.
80-95: Improve sort button accessibility.Expose state via ARIA for assistive tech. Keep the visible text; add
aria-pressedandaria-label.Apply:
- <button + <button onClick={handleClick} className={`flex items-center gap-1 font-semibold transition-colors hover:text-blue-600 ${textAlignClass}`} - title={`Sort by ${label}`} + title={`Sort by ${label}`} + aria-label={`Sort by ${label}`} + aria-pressed={isActive} >frontend/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsx (1)
117-149: Assert the 3‑state cycle and URL updates, not just clickability.Strengthen this test to verify
router.replaceis called with expected?order=values (desc → asc → cleared) for a column, and that default-scoreis restored when cleared.Example:
@@ - sortableColumns.forEach((column) => { - const sortButton = screen.getByTitle(`Sort by ${column}`) - expect(sortButton).toBeInTheDocument() - fireEvent.click(sortButton) - }) + const { useRouter } = jest.requireMock('next/navigation') + const replaceMock = useRouter().replace as jest.Mock + + const sortButton = screen.getByTitle('Sort by Stars') + // unsorted -> desc + fireEvent.click(sortButton) + expect(replaceMock).toHaveBeenLastCalledWith( + expect.stringContaining('order=-stars') + ) + // desc -> asc + fireEvent.click(sortButton) + expect(replaceMock).toHaveBeenLastCalledWith( + expect.stringContaining('order=stars') + ) + // asc -> unsorted (should drop ?order, default to -score) + fireEvent.click(sortButton) + expect(replaceMock).toHaveBeenLastCalledWith( + expect.not.stringContaining('order=') + )Also consider asserting a second column click replaces the previous
orderkey.backend/tests/apps/owasp/api/internal/ordering/project_health_metrics_test.py (1)
29-37: Consider asserting enum types (if exposed) and adding a stability check.If Strawberry exposes an
Orderingenum in Python, prefer that over raw strings. Optionally, add a small dataset test to ensure ordering stabilizes withproject__namewhen primary values are equal.Example:
# if available: from apps.owasp.api.internal.enums import Ordering order_instance = ProjectHealthMetricsOrder(score=Ordering.DESC, ...)Also applies to: 39-43
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
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/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsx(3 hunks)frontend/src/app/projects/dashboard/metrics/page.tsx(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
backend/tests/apps/owasp/api/internal/ordering/project_health_metrics_test.py (3)
backend/apps/owasp/api/internal/ordering/project_health_metrics.py (1)
ProjectHealthMetricsOrder(10-23)backend/apps/owasp/api/internal/nodes/committee.py (3)
stars_count(40-42)forks_count(25-27)contributors_count(15-17)backend/apps/owasp/api/internal/nodes/project_health_metrics.py (1)
created_at(45-47)
backend/apps/owasp/api/internal/ordering/project_health_metrics.py (2)
backend/apps/owasp/api/internal/nodes/committee.py (3)
stars_count(40-42)forks_count(25-27)contributors_count(15-17)backend/apps/owasp/api/internal/nodes/project_health_metrics.py (1)
created_at(45-47)
🔇 Additional comments (3)
backend/tests/apps/owasp/api/internal/ordering/project_health_metrics_test.py (1)
18-26: Field coverage LGTM; aligns with new sortable fields.The expected set matches the ordering type, including
project__nameas a tie‑breaker. Good guard against regressions.backend/apps/owasp/api/internal/ordering/project_health_metrics.py (1)
14-17: Ordering fields addition looks correct and purposeful.New fields (
stars_count,forks_count,contributors_count,created_at) + tie‑breaker comment align with frontend needs and deterministic pagination.Also applies to: 19-23
frontend/src/app/projects/dashboard/metrics/page.tsx (1)
47-53: Review comment is incorrect—field naming is proper.The frontend correctly uses
project_Name, which is the TypeScript-generated equivalent of the backend fieldproject__name. This translation from Python snake_case to camelCase is standard GraphQL code generation behavior. The generated types atfrontend/src/types/__generated__/graphql.ts:478defineproject_Name?: InputMaybe<Ordering>, and the frontend code properly matches this definition. Usingproject__namedirectly would violate the generated type contract and cause GraphQL validation errors.Likely an incorrect or invalid review comment.
|
Test: The linked issue must be assigned to the PR author. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
frontend/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsx (5)
54-70: DRY the next/navigation mocks; add a helper to set search params.You’re reassigning mockSearchParams and re-stubbing useSearchParams in multiple places. Centralize this to reduce duplication and mistakes.
Apply this refactor:
const mockReplace = jest.fn() -let mockSearchParams = new URLSearchParams() +let mockSearchParams = new URLSearchParams() +const setSearchParams = (qs: string) => { + mockSearchParams = new URLSearchParams(qs) + ;(require('next/navigation').useSearchParams as jest.Mock).mockReturnValue(mockSearchParams) +} jest.mock('next/navigation', () => ({ - useSearchParams: jest.fn(() => mockSearchParams), + useSearchParams: jest.fn(() => mockSearchParams), useRouter: jest.fn(() => ({ push: jest.fn(), replace: mockReplace, })), })) describe('MetricsPage', () => { beforeEach(() => { mockReplace.mockClear() - mockSearchParams = new URLSearchParams() - ;(require('next/navigation').useSearchParams as jest.Mock).mockReturnValue(mockSearchParams) + setSearchParams('')Then replace ad-hoc reassignments below with
setSearchParams('order=-stars'), etc.
123-156: Avoid firing events inside waitFor; use userEvent for clicks.Interactions inside waitFor can mask timing issues. Prefer userEvent and assert replace calls per column.
-import { render, screen, waitFor, fireEvent } from '@testing-library/react' +import { render, screen, waitFor } from '@testing-library/react' +import userEvent from '@testing-library/user-event' @@ - test('renders filter dropdown and sortable column headers', async () => { - render(<MetricsPage />) + test('renders filter dropdown and sortable column headers', async () => { + const user = userEvent.setup() + render(<MetricsPage />) @@ - await waitFor(() => { - filterSectionsLabels.forEach((label) => { - expect(screen.getAllByText(label).length).toBeGreaterThan(0) - }) - filterOptions.forEach((option) => { - expect(screen.getAllByText(option).length).toBeGreaterThan(0) - const button = screen.getByRole('button', { name: option }) - fireEvent.click(button) - expect(button).toBeInTheDocument() - }) - - sortableColumns.forEach((column) => { - const sortButton = screen.getByTitle(`Sort by ${column}`) - expect(sortButton).toBeInTheDocument() - fireEvent.click(sortButton) - }) - }) + await waitFor(() => { + filterSectionsLabels.forEach((label) => + expect(screen.getAllByText(label).length).toBeGreaterThan(0) + ) + filterOptions.forEach((option) => + expect(screen.getAllByText(option).length).toBeGreaterThan(0) + ) + sortableColumns.forEach((column) => + expect(screen.getByTitle(`Sort by ${column}`)).toBeInTheDocument() + ) + }) + // Interact after DOM is ready + for (const option of filterOptions) { + await user.click(screen.getByRole('button', { name: option })) + } + for (const column of sortableColumns) { + await user.click(screen.getByTitle(`Sort by ${column}`)) + }Optional: also assert
mockReplacewas called when clicking sortable headers.
115-122: Prefer role-based assertions for table headers.Role=columnheader is more robust than text-only matching.
-headers.forEach((header) => { - expect(screen.getAllByText(header).length).toBeGreaterThan(0) -}) +headers.forEach((header) => { + expect( + screen.getAllByRole('columnheader', { name: header }).length + ).toBeGreaterThan(0) +})Also double-check the label “Health Checked At” vs “Health Check Date” to avoid brittleness.
208-213: Make date formatting deterministic to avoid locale/timezone flakes.Using 'default' locale can fail in CI with non‑en locales/timezones.
- new Date(metric.createdAt).toLocaleString('default', { - month: 'short', - day: 'numeric', - year: 'numeric', - }) + new Date(metric.createdAt).toLocaleDateString('en-US', { + month: 'short', + day: 'numeric', + year: 'numeric', + timeZone: 'UTC', + })Alternatively, compare against a regex or the exact formatter used by the component.
220-233: Assert side effects in the pagination test.Validate that clicking “Next Page” triggers fetchMore (and/or URL update).
const nextPageButton = screen.getByText('Next Page') expect(nextPageButton).toBeInTheDocument() -fireEvent.click(nextPageButton) +fireEvent.click(nextPageButton) +await waitFor(() => { + expect(mockFetchMore).toHaveBeenCalled() +})Optionally assert the variables passed to fetchMore if available.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsx(4 hunks)
🔇 Additional comments (1)
frontend/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsx (1)
157-194: I need to understand how sorting updates the URL and whether pagination is reset. Let me search for the sort handler and how it interacts with the router.Now I need to find where the sort handler is and how it updates the URL query params:
Let me search directly in the codebase for the sort handler and where URL params are built:
Let me search the codebase directly for how the sort handler updates the URL and what URL params it uses:
Based on my investigation, I need to mark this for manual verification because:
The standard Next.js pattern sets
params.set('page', '1')when search or filter changes to reset pagination, which aligns with the review comment's suggestion.However, from the earlier codebase output, I found that pagination uses internal state (
{ offset, limit }), not URL parameters likepage=1. The test setup shows onlyorderparam being used in URL (order=-stars).The review comment suggests adding assertions for
page=1in the URL, but I cannot confirm:
- Whether the sort handler currently implements pagination reset as a URL param
- Whether pagination in this codebase uses URL params or state-based tracking
- What the actual pagination parameter name should be (if
pageis correct)Manually verify the sort handler implementation in
frontend/src/app/projects/dashboard/metrics/page.tsx:
- Find where the sort button click handler calls
router.replace()- Confirm whether it resets pagination (and if so, how it encodes it in URL params)
- Verify the pagination parameter name used in this codebase
- Fix URL/state desync for browser back/forward navigation - Canonicalize invalid order inputs to prevent UI mismatch - Add type-safe FIELD_MAPPING with 'as const' - Improve accessibility with ARIA attributes - Add comprehensive test for 3-state sorting cycle - Regenerate GraphQL types with new sortable fields
EclipseAditya
force-pushed
the
feature/multi-column-sorting
branch
from
a17ffd9 to
0e52842
Compare
|
The linked issue must be assigned to the PR author. |
|
|
oops, messed up here I guess |
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
