Home dashboard UI polish#7902
Conversation
…chart improvements - PostureCard: mono-font score, band tag, clickable alert wrapper opening posture drawer - PostureBreakdownContent: GPS explanation, per-dimension progress bars, clickable rows with Launch icon - DashboardDrawer: dark theme fix via explicit token.colorBgContainer on styles prop - TrendCard: x-axis date labels, per-point tooltips; DSR volume switched to bar chart; Help icon tooltip on titles - PriorityActionsCard: flat list (removed tabs), Status/Severity filter dropdowns, clickable rows - RadarChart: label z-order fix (zIndex=2100), animated cursor tooltip anchored to mouse, score/band tag in labels, text outline (stroke + opacity) - Sparkline/BarChart: x-axis labels with even interior tick spacing, hover tooltips - HomeDashboard: dashed "Last 30 days" divider, ThemeModeToggle icon button, activity/astralis/agent feature flags - SystemCoverageCard/DSRStatusCard: Help icon tooltips on card titles - Remove CommandBar (deleted files + flag) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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
|
Dependency ReviewThe following issues were found:
Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Scanned Files
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## ENG-3353 #7902 +/- ##
===========================================
Coverage ? 85.07%
===========================================
Files ? 627
Lines ? 40794
Branches ? 4742
===========================================
Hits ? 34707
Misses ? 5018
Partials ? 1069 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
kruulik
requested review from
gilluminate
and removed request for
a team and
gilluminate
![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
This is a solid UI polish pass — the shift from tabs to inline filters in PriorityActionsCard, the new score card layout, radar chart tag overlays, sparkline/bar chart tooltips with x-axis labels, and the card-level help tooltips all represent clear improvements. The CSS specificity doubles (.quickActionButton.quickActionButton) to replace !important is a clean technique. The overdue-date normalization in constants.ts is a good correctness fix.
Issues found
Medium — Non-null assertions on tooltipContent prop (BarChart.tsx, Sparkline.tsx)
Both chart components use tooltipContent!(value, label) inside a useCallback. The call site is safe today, but the ! hides the invariant from the type system and will throw at runtime if the callback ever fires unexpectedly. See inline comments for suggested alternatives.
Minor — Internal fidesui/src/ path import (PostureCard.tsx)
Importing directly from fidesui/src/palette/palette.module.scss bypasses the package's public API surface and is fragile against internal refactors or build changes. See inline comment.
Minor — bandColorMap/getBandColor not memoized (PostureBreakdownContent.tsx)
Both are recreated on every render. Wrapping with useMemo/useCallback is straightforward here. See inline comment.
Minor — zIndex={2100} on PolarAngleAxis is a no-op (RadarChart.tsx)
Recharts doesn't honour a zIndex prop on SVG elements — DOM paint order is the correct lever. See inline comment.
Observation — Mixed-timezone arithmetic in due-date normalization (constants.ts)
The fix is correct for its purpose; a short comment explaining the UTC-parse / local-mutate behaviour would help future readers. See inline comment.
🔬 Codegraph: connected (46300 nodes)
💡 Write /code-review in a comment to re-run this review.
| fontSize: token.fontSizeSM, | ||
| }} | ||
| > | ||
| {tooltipContent!(entry.value, entry.label)} |
There was a problem hiding this comment.
clients/fidesui/src/components/charts/BarChart.tsx:129
Non-null assertion tooltipContent! is used here, but the outer condition (showTooltip || tooltipContent) doesn't guarantee tooltipContent is defined when renderTooltipContent is called via content={tooltipContent ? renderTooltipContent : undefined}. The logic is actually safe in practice (the content prop is only set when tooltipContent is truthy), but the assertion is fragile and hides the intent.
Consider a guard instead:
{tooltipContent(entry.value, entry.label)}Since renderTooltipContent is only ever set as the content prop when tooltipContent is defined, you can simply call it directly (no !) and TypeScript will be happy after narrowing the closure capture — or restructure the useCallback so tooltipContent is required in its type signature.
| top: strokeWidth, | ||
| right: 20, | ||
| right: 0, | ||
| bottom: bottomMargin, |
There was a problem hiding this comment.
clients/fidesui/src/components/charts/Sparkline.tsx:133
Same non-null assertion pattern as in BarChart.tsx — tooltipContent!(entry.value, label). The callback is only registered when tooltipContent is defined (!empty && tooltipContent && <Tooltip ...>), so it's safe, but the ! makes the type contract invisible to readers and linters.
Consider either:
- Narrowing the type: move the
tooltipContentcheck into a helper that returnsNonNullable<typeof tooltipContent>. - Simply using
tooltipContent?.(entry.value, label)(optional call) so the worst case isundefinedrendered as nothing rather than a runtime throw.
| Tooltip, | ||
| useThemeMode, | ||
| } from "fidesui"; | ||
| import palette from "fidesui/src/palette/palette.module.scss"; |
There was a problem hiding this comment.
clients/admin-ui/src/home/PostureCard.tsx:14
import palette from "fidesui/src/palette/palette.module.scss";This reaches into fidesui's internal src/ tree rather than going through the package's public API. It will break if fidesui's internal directory structure changes, if the package starts bundling differently, or when used in an environment that resolves fidesui from a compiled artifact rather than source.
If the palette tokens need to be consumed externally, the cleaner path is to export them from fidesui's index (e.g., import { palette } from "fidesui") or convert the values to CSS custom properties that can be read at runtime from the token instead.
| {!empty && ( | ||
| <PolarAngleAxis | ||
| dataKey="subject" | ||
| zIndex={2100} |
There was a problem hiding this comment.
clients/fidesui/src/components/charts/RadarChart.tsx:350
zIndex={2100} is not a valid prop for Recharts' PolarAngleAxis. Recharts SVG elements don't honour a zIndex prop — SVG layering is determined by DOM order, not CSS z-index. This prop will be silently ignored (or generate an "unknown DOM attribute" React warning in dev mode).
If the goal is to ensure tick labels render above the radar fill/grid, the correct approach is to move the PolarAngleAxis element later in the JSX tree so it's painted last by SVG's painter's algorithm.
There was a problem hiding this comment.
This is overall looking very polished! Much better than what I had.
I like the new priority actions status + severity filter, and how you've styled the timeline charts.
3 small things I'd like to see, but let me know what is feasible so we can merge by EOD today:
- Governance posture score card - I'd like to see a CTA like "View Details ->" as it's not obvious that section is clickable. Instead of the entire "Up 3.8 points this week, driven by Casey completing 34 classification reviews across Snowflake and BigQuery.", I think we can truncate to "Up 3.8 points this week." with a small green up or red down arrow in front of it to indicate the trend.
- The date text on the DSR volume graph are overlapping, so let's check this.
- Dark mode causes the highlighted dropdown filter text to be illegible:
There was a problem hiding this comment.
Code Review — Home Dashboard UI Polish
Overall this is a well-structured polish pass. The feature-flag gating of alpha cards, the migration from tabs to inline filter dropdowns, the radar chart label z-ordering fix, and the tooltip-on-chart-title pattern are all solid. A few items worth addressing before merge:
Issues found
Medium
PostureCard.tsx:14— Direct import fromfidesui/src/palette/palette.module.scssbypasses the package's public API. This is fragile; prefer re-exporting the tokens fromfidesui's public index or using Ant Design CSS variables already on the token object.
Low / Nit
PostureCard.tsxclickable div —aria-label="View posture breakdown"was removed from therole="button"div. Screen-reader users now land on an unlabeled interactive control. Should be restored.BarChart.tsx/Sparkline.tsx— BothrenderTooltipContentcallbacks use the non-null assertiontooltipContent!. The invariant is upheld by the surrounding conditional, but optional chaining (tooltipContent?.(...)) is safer and signals intent more clearly.constants.tsgetUrgencyGroup—setHours(23,59,59,999)normalises to the local browser timezone, whilenew Date("YYYY-MM-DD")is parsed as UTC midnight. The fix improves things for timezones east of UTC but may still be off for users west of UTC. A pure string comparison (dueDate < new Date().toISOString().slice(0,10)) is unambiguous if sub-day precision isn't needed.PostureBreakdownContent.tsx—bandColorMapis an object literal created unconditionally on every render; auseMemodepending ontokenwould be more consistent.RadarChart.tsxtooltipwidth: 140— Fixed width can clip longer dimension descriptions.maxWidth: 200would be more flexible.
Informational
AgentBriefingBanner.tsx—fixPluralGrammarwas a client-side workaround for server-generated text ("1 privacy requests are overdue"). Confirm the server now handles this correctly before merging, so the regression isn't invisible.
Positive highlights
- The
.quickActionButton.quickActionButtonCSS specificity bump to avoid!importantis the right approach. - Moving x-axis tick rendering to a custom
renderXAxisTickcallback (Sparkline) andrenderSimpleTick(BarChart) with properidx-based addressing is a clean fix for the earlier label-offset bug. aspect-ratio: 1on.radarChartWrapperis a good constraint to prevent the radar from distorting at extreme card heights.- Feature-flag gating of
ActivityFeedCard,AstralisPanel, andAgentBriefingBannerwith proper column-width fallback when only one panel is enabled is well-handled.
🔬 Codegraph: connected (46300 nodes)
💡 Write /code-review in a comment to re-run this review.
| Tooltip, | ||
| useThemeMode, | ||
| } from "fidesui"; | ||
| import palette from "fidesui/src/palette/palette.module.scss"; |
There was a problem hiding this comment.
clients/admin-ui/src/home/PostureCard.tsx:14
Direct import from fidesui/src/... reaches into the package's internal source tree, bypassing its public API:
import palette from "fidesui/src/palette/palette.module.scss";If fidesui ever restructures its internals (path rename, file split, build output change) this will break silently. Consider re-exporting the specific palette tokens you need from fidesui's public index, or using the CSS custom properties (var(--...)) already exposed on palette.FIDESUI_BG_MINOS / palette.FIDESUI_MINOS via the theme tokens instead.
| borderRadius: token.borderRadius, | ||
| padding: `${token.paddingXXS}px ${token.paddingXS}px`, | ||
| boxShadow: token.boxShadow, | ||
| fontSize: token.fontSizeSM, |
There was a problem hiding this comment.
clients/fidesui/src/components/charts/BarChart.tsx:126
The non-null assertion tooltipContent! here is safe in practice (the outer (showTooltip || tooltipContent) && guard prevents this callback from being registered without a value), but the assertion is invisible to readers and will panic if the invariant is ever broken. Prefer an explicit guard or optional call:
{tooltipContent?.(entry.value, entry.label)}Same pattern in Sparkline.tsx (the renderTooltipContent callback there also uses tooltipContent!).
| <Icons.Help size={14} className="opacity-30" /> | ||
| </Flex> | ||
| </Tooltip> | ||
| } |
There was a problem hiding this comment.
clients/admin-ui/src/home/PostureCard.tsx:116
The aria-label that was previously on this role="button" div (aria-label="View posture breakdown") was removed in this PR. Screen-reader users navigating by button now land on an unnamed control. Add it back:
<div
role="button"
tabIndex={0}
aria-label="View posture breakdown"
className={styles.clickableAlert}
...
>| // Past due date → Overdue, regardless of severity | ||
| if (dueDate && new Date(dueDate) < new Date()) { | ||
| return "overdue"; | ||
| // Normalize to end-of-day to avoid timezone boundary issues with date-only strings |
There was a problem hiding this comment.
clients/admin-ui/src/features/dashboard/constants.ts:156
setHours(23, 59, 59, 999) normalises to local browser timezone end-of-day, while new Date("2026-04-13") (a date-only ISO string) is parsed as UTC midnight. This fixes the most common off-by-one in timezones east of UTC, but a user in UTC+2 whose local midnight is 22:00 UTC would still see "overdue" an hour early.
If dueDate is always a date-only string (YYYY-MM-DD) with no intent for sub-day precision, a purely string-based comparison (dueDate < today) avoids all timezone ambiguity and is simpler:
if (dueDate) {
const today = new Date().toISOString().slice(0, 10); // "YYYY-MM-DD" in UTC
if (dueDate < today) return "overdue";
}If local-timezone semantics are intentional, the current approach is acceptable — just worth a comment clarifying why setHours is used vs. a UTC comparison.
|
|
||
| const { briefing: rawText, quick_actions: quickActions } = briefing; | ||
| const text = fixPluralGrammar(rawText); | ||
| const text = rawText; |
There was a problem hiding this comment.
clients/admin-ui/src/home/AgentBriefingBanner.tsx:53
fixPluralGrammar was a client-side workaround for server-generated text that said "1 privacy requests are overdue". Removing it is fine if the server now generates grammatically correct text, but if it doesn't this will regress visible grammar errors. Worth a quick sanity-check with a real briefing payload containing a count of 1 before merging.
| const bandColorMap: Record<string, string> = { | ||
| success: token.colorSuccess, | ||
| caution: token.colorWarning, | ||
| error: token.colorError, |
There was a problem hiding this comment.
clients/admin-ui/src/home/PostureBreakdownContent.tsx:37
bandColorMap is a plain object literal created inside the component body — it re-allocates on every render. It only depends on token values (which are stable between renders unless the theme changes), so either:
- Wrap it in
useMemo(() => ({ ... }), [token]), or - Move
getBandColoroutside the component and accepttokenas a parameter.
Low impact for a drawer-level component, but worth keeping consistent with the memoization pattern used elsewhere in this file.
| @@ -331,6 +375,7 @@ export const RadarChart = ({ | |||
| transform: "translate(-50%, -100%) translateY(-8px)", | |||
There was a problem hiding this comment.
clients/fidesui/src/components/charts/RadarChart.tsx:375
The tooltip container is given a fixed width: 140. Descriptions like "Percentage of systems with at least one data classification applied" (from DIMENSION_DESCRIPTIONS) can easily overflow or wrap awkwardly at 140 px. Consider using maxWidth instead (e.g., maxWidth: 200) so short tooltips stay compact while longer ones can expand.
2 participants
Ticket ENG-3353
Description Of Changes
Polish pass on the new home dashboard: PostureCard, TrendCard, PriorityActionsCard, and shared chart primitives.
Code Changes
Steps to Confirm
Pre-Merge Checklist
CHANGELOG.mdupdated