Skip to content

Commit 0e52842

Browse files
committed
fix: address CodeRabbit feedback for multi-column sorting
- 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
1 parent 614e271 commit 0e52842

File tree

3 files changed

+72
-6
lines changed

3 files changed

+72
-6
lines changed

frontend/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsx

Lines changed: 47 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -143,10 +143,56 @@ describe('MetricsPage', () => {
143143
sortableColumns.forEach((column) => {
144144
const sortButton = screen.getByTitle(`Sort by ${column}`)
145145
expect(sortButton).toBeInTheDocument()
146-
fireEvent.click(sortButton)
147146
})
148147
})
149148
})
149+
150+
test('handles sorting state and URL updates', async () => {
151+
const mockReplace = jest.fn()
152+
const { useRouter, useSearchParams } = jest.requireMock('next/navigation')
153+
;(useRouter as jest.Mock).mockReturnValue({
154+
push: jest.fn(),
155+
replace: mockReplace,
156+
})
157+
158+
// Test unsorted -> descending
159+
;(useSearchParams as jest.Mock).mockReturnValue(new URLSearchParams())
160+
const { rerender } = render(<MetricsPage />)
161+
162+
const sortButton = screen.getByTitle('Sort by Stars')
163+
fireEvent.click(sortButton)
164+
165+
await waitFor(() => {
166+
expect(mockReplace).toHaveBeenCalledWith(expect.stringContaining('order=-stars'))
167+
})
168+
169+
// Test descending -> ascending
170+
mockReplace.mockClear()
171+
;(useSearchParams as jest.Mock).mockReturnValue(new URLSearchParams('order=-stars'))
172+
rerender(<MetricsPage />)
173+
174+
const sortButtonDesc = screen.getByTitle('Sort by Stars')
175+
fireEvent.click(sortButtonDesc)
176+
177+
await waitFor(() => {
178+
const lastCall = mockReplace.mock.calls[mockReplace.mock.calls.length - 1][0]
179+
expect(lastCall).toContain('order=stars')
180+
expect(lastCall).not.toContain('order=-stars')
181+
})
182+
183+
// Test ascending -> unsorted (removes order param, defaults to -score)
184+
mockReplace.mockClear()
185+
;(useSearchParams as jest.Mock).mockReturnValue(new URLSearchParams('order=stars'))
186+
rerender(<MetricsPage />)
187+
188+
const sortButtonAsc = screen.getByTitle('Sort by Stars')
189+
fireEvent.click(sortButtonAsc)
190+
191+
await waitFor(() => {
192+
const lastCall = mockReplace.mock.calls[mockReplace.mock.calls.length - 1][0]
193+
expect(lastCall).not.toContain('order=')
194+
})
195+
})
150196
test('render health metrics data', async () => {
151197
render(<MetricsPage />)
152198
const metrics = mockHealthMetricsData.projectHealthMetrics

frontend/src/app/projects/dashboard/metrics/page.tsx

Lines changed: 21 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -17,13 +17,15 @@ import ProjectsDashboardDropDown from 'components/ProjectsDashboardDropDown'
1717

1818
const PAGINATION_LIMIT = 10
1919

20-
const FIELD_MAPPING: Record<string, string> = {
20+
const FIELD_MAPPING = {
2121
score: 'score',
2222
stars: 'starsCount',
2323
forks: 'forksCount',
2424
contributors: 'contributorsCount',
2525
createdAt: 'createdAt',
26-
}
26+
} as const
27+
28+
type OrderKey = keyof typeof FIELD_MAPPING
2729

2830
const parseOrderParam = (orderParam: string | null) => {
2931
if (!orderParam) {
@@ -32,10 +34,13 @@ const parseOrderParam = (orderParam: string | null) => {
3234

3335
const isDescending = orderParam.startsWith('-')
3436
const fieldKey = isDescending ? orderParam.slice(1) : orderParam
35-
const graphqlField = FIELD_MAPPING[fieldKey] || 'score'
37+
const isValidKey = fieldKey in FIELD_MAPPING
38+
const normalizedKey = isValidKey ? fieldKey : 'score'
39+
const graphqlField = FIELD_MAPPING[normalizedKey as OrderKey]
3640
const direction = isDescending ? Ordering.Desc : Ordering.Asc
41+
const normalizedUrlKey = direction === Ordering.Desc ? `-${normalizedKey}` : normalizedKey
3742

38-
return { field: graphqlField, direction, urlKey: orderParam }
43+
return { field: graphqlField, direction, urlKey: normalizedUrlKey }
3944
}
4045

4146
const buildGraphQLOrdering = (field: string, direction: Ordering) => {
@@ -53,7 +58,7 @@ const buildOrderingWithTieBreaker = (primaryOrdering: Record<string, Ordering>)
5358
]
5459
const SortableColumnHeader: FC<{
5560
label: string
56-
fieldKey: string
61+
fieldKey: OrderKey
5762
currentOrderKey: string
5863
onSort: (orderKey: string | null) => void
5964
align?: 'left' | 'center' | 'right'
@@ -83,6 +88,8 @@ const SortableColumnHeader: FC<{
8388
onClick={handleClick}
8489
className={`flex items-center gap-1 font-semibold transition-colors hover:text-blue-600 ${textAlignClass}`}
8590
title={`Sort by ${label}`}
91+
aria-label={`Sort by ${label}`}
92+
aria-pressed={isActive}
8693
>
8794
<span className="truncate">{label}</span>
8895
<FontAwesomeIcon
@@ -171,6 +178,15 @@ const MetricsPage: FC = () => {
171178
},
172179
})
173180

181+
useEffect(() => {
182+
const { field: f, direction: d } = parseOrderParam(searchParams.get('order'))
183+
const nextOrdering = buildGraphQLOrdering(f, d)
184+
if (JSON.stringify(nextOrdering) !== JSON.stringify(ordering)) {
185+
setOrdering(nextOrdering)
186+
}
187+
// eslint-disable-next-line react-hooks/exhaustive-deps
188+
}, [searchParams])
189+
174190
useEffect(() => {
175191
if (data) {
176192
setMetrics(data.projectHealthMetrics)

frontend/src/types/__generated__/graphql.ts

Lines changed: 4 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)