Fix Project Metrics dashboard responsiveness on small screens#3412
Fix Project Metrics dashboard responsiveness on small screens#3412kasya merged 13 commits intoOWASP:mainfrom
Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughRefactors projects metrics UI for responsive/mobile use, replaces sortable column headers with a Sort-By dropdown and ORDERING_MAP, updates MetricsCard layout and score badge styling, and updates multiple E2E and unit tests to match the new DOM and selectors. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/__tests__/unit/components/MetricsCard.test.tsx (1)
68-73: Inconsistent query pattern may cause test failures.This test still uses
getByText(score.toString())while other assertions were updated togetAllByText(...)[0]. If MetricsCard now renders the score in both mobile and desktop layouts,getByTextwill throw when multiple matches exist. The subsequent.closest('div')call would also select an unpredictable element if multiple scores are present.Proposed fix
for (const [score, expectedClass] of cases) { const metric = makeMetric({ score }) render(<MetricsCard metric={metric} />) - const scoreEl = screen.getByText(score.toString()).closest('div') + const scoreEl = screen.getAllByText(score.toString())[0].closest('div') expect(scoreEl).toHaveClass(expectedClass) }
🤖 Fix all issues with AI agents
In `@frontend/__tests__/e2e/pages/ProjectsHealthDashboardMetrics.spec.ts`:
- Around line 56-69: The test uses toLocaleString('default', ...) which causes
locale-dependent flakiness; update the expectation to use a fixed explicit
locale matching the MetricsCard (e.g., 'en-US' or the locale used by
MetricsCard) when formatting new Date(firstMetric.createdAt).toLocaleString so
the string comparison is deterministic across CI environments; locate the
assertion building the selector (the page.getByRole('link')...filter(...) that
references new Date(firstMetric.createdAt).toLocaleString) and replace 'default'
with the explicit locale.
🧹 Nitpick comments (5)
frontend/src/app/projects/dashboard/metrics/page.tsx (2)
76-81: Consider aligning mobile sort behavior with desktop for consistency.The
getNextOrderKeyhelper toggles between descending and ascending only. Desktop sorting (viaSortableColumnHeader) has a three-state cycle: unsorted → descending → ascending → unsorted (clearing on third click). Mobile users must explicitly use "Clear sorting" to reset.This UX difference may be intentional, but verify it matches the expected behavior. If three-state cycling is desired for mobile, the logic would need adjustment.
296-331: Mobile sort dropdown implementation looks good.The mobile-only sort UI (
sm:hidden) with clear sorting action provides a usable experience on small screens. The icon dynamically reflects sort direction, and selectedLabels displays the current sort field.Minor observation: When
urlKeyis falsy but a default sort (-score) is applied internally, the dropdown shows no selected label, which may be slightly confusing. Consider showing "Score" as the default selected label for clarity.frontend/src/components/MetricsCard.tsx (1)
53-109: Well-structured mobile layout that addresses the PR objective.The mobile card design provides good information hierarchy with the score badge prominently displayed alongside the project name, followed by key metrics in a grid. The stacked layout works well for narrow screens.
Consider extracting the score color logic (lines 69-73 and 44-47) into a shared helper to reduce duplication, though this is a minor maintainability improvement.
♻️ Optional: Extract score color logic
// At the top of the file or in a shared utils file const getScoreColorClasses = (score: number) => ({ 'bg-green-500': score >= 75, 'bg-orange-500': score >= 50 && score < 75, 'bg-red-500': score < 50, })Then use in both layouts:
- { - 'bg-green-500': metric.score >= 75, - 'bg-orange-500': metric.score >= 50 && metric.score < 75, - 'bg-red-500': metric.score < 50, - } + getScoreColorClasses(metric.score)frontend/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsx (1)
244-262: Test selector update accommodates the new responsive layout.Using
getAllByText(...)[0]is a reasonable approach given that the responsive changes render duplicate text in both desktop and mobile layouts. The tests continue to verify that the expected data is present in the document.For future maintainability, consider adding
data-testidattributes to specific elements if more precise targeting becomes necessary, but this is acceptable for now.frontend/__tests__/e2e/pages/ProjectsHealthDashboardMetrics.spec.ts (1)
32-77: Assertions are broad—consider verifying co-location of values.Each assertion independently checks that some link contains the expected text. If multiple metric cards exist and happen to share a value (e.g., two projects with the same fork count), these assertions pass without confirming the values belong to the same card. For stronger guarantees, consider asserting that all expected values appear within the same link element:
await expect(metricsLink).toContainText(firstMetric.starsCount.toString()) await expect(metricsLink).toContainText(firstMetric.forksCount.toString()) // etc.This would verify the first metric card contains all expected values together.
|
Hi @kasya I’ve added a new card-based layout for the mobile view since the table layout doesn’t really work well on smaller screens and one more thing like while implementing sorting for mobile view i got a bit unsure about the UI. Right now, when a user taps on a field like Stars, it sorts in descending order first, and tapping it again switches to ascending. |
|
@kasya @arkid15r Hi, I’ve created a separate issue for this #3445 about the Project Health Dashboard if you feel this should be handled here instead i will work on it in this pr itself |
There was a problem hiding this comment.
Thanks for working on this. Also I think it is better to make the same design for all screens instead of having a table for large screens and a list of cards for mobile screens, we should use your new design for all screens. With this, not also the user will see the same UI/UX in different devices, but the code will be cleaner and more simple. I will leave the decision of changing the design of large screens to @kasya. Please see the below comments.
anurag2787
force-pushed
the
fix/responsive-project-matrics
branch
from
a96cbea to
075698e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/src/app/projects/dashboard/metrics/page.tsx`:
- Around line 299-330: The icon selection currently uses urlKey?.startsWith('-')
which no longer matches new keys like scoreDesc/starsAsc; update the icon prop
on ProjectsDashboardDropDown to derive direction from the urlKey suffix (e.g.,
use urlKey?.endsWith('Desc') to pick FaArrowDownWideShort, otherwise
FaArrowUpWideShort) and ensure it falls back correctly when urlKey is
null/empty; reference urlKey, MOBILE_SORT_FIELDS and handleSort when making the
change so mobile sort display and behaviour remain consistent.
🧹 Nitpick comments (1)
frontend/__tests__/unit/pages/ProjectsHealthDashboardMetrics.test.tsx (1)
60-286: Consider adding test coverage for the mobile sort dropdown.The test file has been updated for the new ordering keys, but there's no explicit test verifying the new mobile "Sort By" dropdown functionality. Consider adding a test that verifies:
- The mobile sort dropdown renders with correct options
- Selecting a sort option triggers
handleSortwith the appropriate key- The "Reset Sorting" option calls
handleSort(null)This would help ensure the new mobile sorting feature works as expected.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/src/app/projects/dashboard/metrics/page.tsx`:
- Around line 58-66: The parseOrderParam function currently uses the `in`
operator against MOBILE_ORDERING_MAP which allows prototype keys (e.g.,
"toString") to pass validation; change the check to an own-property check (e.g.,
Object.prototype.hasOwnProperty.call(MOBILE_ORDERING_MAP, orderParam)) so only
true keys in MOBILE_ORDERING_MAP are accepted, then safely read { field,
direction } from MOBILE_ORDERING_MAP[orderParam as MobileOrderingKey] and fall
back to the default when the key is not an own property or values are missing.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/src/app/projects/dashboard/metrics/page.tsx`:
- Around line 300-329: The dropdown was changed to selectionMode="single", which
breaks previous multi-criteria sorting support; either revert selectionMode back
to "multiple" on ProjectsDashboardDropDown and ensure onAction/handleSort and
MOBILE_ORDERING_MAP accept an array of keys (update handleSort to accept
multiple keys and apply MOBILE_ORDERING_MAP accordingly), or explicitly keep
selectionMode="single" but then update handleSort and MOBILE_ORDERING_MAP to
only accept a single key and remove any multi-sort assumptions (choose one
consistent approach between ProjectsDashboardDropDown, handleSort,
MOBILE_ORDERING_MAP, and MOBILE_SORT_FIELDS).
I agree! I like this new design 👍🏼 Let's make it the same for all screen sizes. |
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/src/components/MetricsCard.tsx`:
- Around line 66-72: The date rendering in the MetricsCard component uses
toLocaleString('default', ...) which can differ between server and client and
cause hydration mismatches; update the rendering to use a fixed locale (e.g.,
replace 'default' with 'en-US') or move the formatting to client-only rendering
(e.g., compute formattedDate in a useEffect/useState) so the output is
deterministic; target the JSX that formats metric.createdAt in MetricsCard and
ensure the change uses a stable locale or client-side formatting.
🧹 Nitpick comments (2)
frontend/src/components/MetricsCard.tsx (2)
36-36: Minor: Redundant margin classes.The
my-4on the<hr>may create extra spacing since the parent usesspace-y-4which already adds margin between children. Consider removingmy-4if the spacing looks excessive.
14-21: Consider using a falsy check instead of strict empty-string comparison.The
metric.projectNamefield is optional (projectName?: stringin HealthMetricsProps), meaning it can beundefined. The strict equality checkmetric.projectName === ''only handles empty strings, leavingundefinedvalues unhandled—the "No name" fallback won't display for undefined values.♻️ Suggested fix
<p className={clsx( 'text-md truncate font-semibold lg:text-xl', - metric.projectName === '' ? 'text-gray-500' : 'text-gray-800 dark:text-gray-200' + !metric.projectName ? 'text-gray-500' : 'text-gray-800 dark:text-gray-200' )} > - {metric.projectName === '' ? 'No name' : metric.projectName} + {metric.projectName || 'No name'} </p>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3412 +/- ##
==========================================
- Coverage 85.61% 85.48% -0.13%
==========================================
Files 461 461
Lines 14254 14240 -14
Branches 1905 1896 -9
==========================================
- Hits 12203 12173 -30
- Misses 1672 1688 +16
Partials 379 379
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 3 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
@anurag2787 this looks good, thank you! 👍🏼
I pushed a little change to update card styling for larger screens and how we show Time health was checked - with that change the card is smaller on larger screens and doesn't have too much empty space.
3 participants
Proposed change
This PR improves the responsiveness of the Project Metrics dashboard on smaller screens. It updates the layout to adapt properly on mobile by replacing table-style views with responsive cards, improving spacing and readability, and adding a mobile-friendly sorting control.
Resolves #3368
Before
After
Checklist
make check-testlocally: all warnings addressed, tests passed