ENG-3353 Homepage dashboard UI polish#7904
Conversation
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
|
eastandwestwind
requested review from
gilluminate
and removed request for
a team
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review — Home Dashboard UI Polish
Overall this is a solid UI polish pass. The layout changes (removing CommandBar, adding feature flags to ActivityFeedCard/AstralisPanel, replacing urgency tabs with dropdowns, adding progress bars to the posture breakdown, and the new custom radar tick tooltips) are well-structured. The chart primitives (Sparkline, BarChart) get useful new capabilities (x-axis labels, tooltip content) cleanly. A few things worth addressing:
Issues
Accessibility regression (medium)
The role="button" div in PostureCard lost its aria-label="View posture breakdown" during refactoring. Screen readers will have nothing to announce for this interactive element. See inline comment.
Internal fidesui import (medium)
PostureCard.tsx imports directly from fidesui/src/palette/palette.module.scss. This bypasses the package's public API and will break if the internal directory structure changes. See inline comment.
Non-null assertion risk in BarChart (medium)
tooltipContent!(...) is called inside a callback that's also activated by showTooltip=true. If showTooltip is true but tooltipContent is undefined, this will throw. See inline comment.
Invalid HTML: <a> wrapping <li> in PriorityActionsCard (medium)
NextLink (renders as <a>) wraps List.Item (renders as <li>), producing invalid HTML. Browsers are tolerant but assistive technologies and validators may not be. See inline comment.
ThemeModeToggle gating inconsistency (low)
The toggle is now unconditionally rendered in HomeDashboard but the component file still carries a "temporary, only for testing in dev" comment. Needs either a feature flag or the comment removed. See inline comment.
Dead-code alias in AgentBriefingBanner (low)
const text = rawText; is a no-op left over from an incomplete rename. See inline comment.
Hardcoded tooltip width in RadarChart (low)
width: 140 is too narrow for some of the dimension descriptions now being passed via tooltipContent. See inline comment.
Positive notes
normalizeSlaHealthinDSRStatusCardis a clean defensive guard — good call ensuring all four DSR types always have entries before they reach the chart.- The new
getUrgencyGrouplogic (past-due → overdue, regardless of severity) is a clear semantic improvement over the old logic that bucketed all items with due dates as "scheduled". - The custom tick tooltip in
RadarChart(positioning viagetBoundingClientRectrather than relying on Recharts' built-inTooltip) sidesteps a known clipping issue with SVG tooltips insideResponsiveContainer— good approach. - The
buildDateLabelshelper inTrendCardhandles thepointCount === 1edge case correctly via(pointCount - 1 || 1).
🔬 Codegraph: connected (46363 nodes)
💡 Write /code-review in a comment to re-run this review.
| @@ -104,19 +131,41 @@ export const PostureCard = () => { | |||
| } | |||
There was a problem hiding this comment.
clients/admin-ui/src/home/PostureCard.tsx:131
The aria-label that was on this div (aria-label="View posture breakdown") was removed in this refactor. Without it, screen readers have no way to announce the purpose of this interactive element. Since the div has role="button" behavior (click + keydown handlers) but isn't a <button>, the accessible label is especially important.
<div
role="button"
tabIndex={0}
aria-label="View posture breakdown" // ← restore this
className={styles.clickableAlert}
...
>| subject: dimension.label, | ||
| value: dimension.score, | ||
| subject: DIMENSION_LABELS[dimension.dimension] ?? dimension.label, | ||
| value: Math.round(dimension.score), |
There was a problem hiding this comment.
clients/admin-ui/src/home/PostureCard.tsx:63
This imports directly from fidesui/src/palette/palette.module.scss — reaching into the package's internal src directory rather than its public API surface. If fidesui's internal structure ever changes this path will break silently at build time. Consider whether the palette values can be accessed via the Ant theme token system (antTheme.useToken()) or exposed as a proper export from fidesui instead.
| @@ -97,26 +169,38 @@ export const BarChart = ({ | |||
| <XAxis | |||
There was a problem hiding this comment.
clients/fidesui/src/components/charts/BarChart.tsx:169
tooltipContent!(entry.value, entry.label) uses a non-null assertion. The renderTooltipContent callback is registered when showTooltip || tooltipContent is true (line ~206), which means it could be called when showTooltip=true but tooltipContent is undefined. The non-null assertion will then throw at runtime.
Either guard with a conditional:
{tooltipContent ? tooltipContent(entry.value, entry.label) : null}Or ensure renderTooltipContent is only ever registered when tooltipContent is defined.
| @@ -128,39 +163,32 @@ export const PriorityActionsCard = () => { | |||
| size="small" | |||
There was a problem hiding this comment.
clients/admin-ui/src/home/PriorityActionsCard.tsx:163
Wrapping <List.Item> (which renders as <li>) inside a NextLink (which renders as <a>) produces invalid HTML — <a> is an inline element and the HTML spec does not allow <li> as a descendant of <a>. While browsers are generally permissive about this, it can cause unexpected DOM structure, break assistive technologies, and potentially fail HTML validation in CI.
A common alternative is to make the list item use a relative container and position an absolutely-stretched anchor inside it, or convert the entire list to use div-based rows rather than ul/li semantics.
There was a problem hiding this comment.
Wrapping List.Item contents inside NextLink breaks Ant Design's internal flex layout (List.Item.Meta needs to be a direct child of List.Item). The current structure works correctly in practice — skipping this one.
|
|
||
| if (!briefing) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
clients/admin-ui/src/home/AgentBriefingBanner.tsx:50
const text = rawText; is a dead-code no-op — text is assigned the same value as rawText and neither transformation nor processing happens. The text variable is then used below where rawText could be used directly. This looks like a leftover from an incomplete refactor (perhaps the intent was to process/sanitize the text). Either remove the alias and use rawText (or the original briefing name), or add the intended transformation.
| return ( | ||
| <div | ||
| className={classNames("w-full h-full", className, { | ||
| ref={containerRef} |
There was a problem hiding this comment.
clients/fidesui/src/components/charts/RadarChart.tsx:289
The tooltip container has width: 140 hardcoded. Some dimension descriptions passed via tooltipContent (e.g. "Alignment between collected consent records and actual data processing activities" — 66 characters) can easily overflow or get heavily wrapped at 140px. Consider either setting a maxWidth and allowing the tooltip to auto-size, or using a wider minimum such as min-width: 140, max-width: 220.
- Replace non-null assertion with optional chaining in BarChart tooltipContent - Remove dead-code alias in AgentBriefingBanner - Gate ThemeModeToggle behind alphaDarkMode feature flag Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2 participants
https://ethyca.atlassian.net/browse/ENG-3353
Description Of Changes
This PR delivers a comprehensive UI polish pass on the Fides Admin home dashboard. It refines the posture score card, trend charts, priority actions list, radar chart, and several supporting components to improve visual clarity, interactivity, and dark mode support. It also gates several alpha-stage dashboard widgets (Activity Feed, Astralis card, Agent Briefing banner) behind feature flags.
Code Changes
HomeDashboard.tsx,HomeContainer.tsx): RemovedCommandBar, added dark mode toggle, wrapped alpha widgets behind feature flags, adjusted grid proportions.PostureCard.tsx,PostureBreakdownContent.tsx): Redesigned score display, added help tooltips, progress bars, clickable dimension links in drawer.PriorityActionsCard.tsx,constants.ts): Replaced tab-based urgency groups with dropdown filters; refactored urgency logic (act_now→overdue,scheduled→urgent,when_ready→pending).DSRStatusCard.tsx): Added help tooltip, renamed SLA label, redesigned overdue indicator as badge, normalized SLA health data.TrendCard.tsx): Added x-axis date labels, hover tooltips, bar chart variant for DSR volume.Sparkline.tsx,BarChart.tsx,RadarChart.tsx): Added tooltip support, x-axis label rendering, inline score tags on radar, dark mode select tokens.flags.json): AddedalphaDashboardActivityFeed,alphaDashboardAstralisCard,alphaDashboardAgentBriefing.consent_module_enabledfrom backend config (application_config.py,admin_ui_settings.py,utils.py), frontend nav config, and related tests.ProgressCard/utils.ts,MonitorStatsWidget.tsx,MonitorProgressWidget.tsx): Fixed progress calculations, simplified stat rendering, removed unnecessary refetch/loading logic.Steps to Confirm
Pre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works