Skip to content

ENG-3036: Dashboard UI polish#7758

Merged
kruulik merged 29 commits intomainfrom
dashboard-ui-polish
Mar 26, 2026
Merged

ENG-3036: Dashboard UI polish#7758
kruulik merged 29 commits intomainfrom
dashboard-ui-polish

Conversation

@kruulik
Copy link
Copy Markdown
Contributor

@kruulik kruulik commented Mar 25, 2026

Ticket ENG-3036

Description Of Changes

Miscellaneous UI polish based on design feedback, as well as small code refactors.

Key changes:

  • Route @carbon/icons-react and antd imports through fidesui re-exports in admin-ui components
  • Reduce CustomCard title and tab font sizes using antd design tokens (token.fontSize, token.fontSizeSM)
  • Improve RadarChart empty state (show grid only), extract catmullRomClosedPath utility to chart-utils with division-by-zero guard
  • Make AgentBriefingBanner self-contained (owns its own data fetching and dismissal state)
  • Replace <Spin> wrappers with Card loading prop across dashboard cards
  • Remove commented-out debug code, update stale docstrings
image

Code Changes

  • clients/fidesui/src/hoc/CustomCard.tsx - Reduce title font to token.fontSize, wrap tab labels at token.fontSizeSM
  • clients/fidesui/src/hoc/CustomCard.module.scss - Add inline header tab padding adjustments
  • clients/fidesui/src/components/charts/RadarChart.tsx - Empty state with grid-only rendering, extract WeightShape component
  • clients/fidesui/src/components/charts/chart-utils.ts - Add catmullRomClosedPath with epsilon guard
  • clients/fidesui/src/components/charts/RadarChart.stories.tsx - Remove click wrapper, add tooltip story
  • clients/fidesui/src/components/charts/RadarTooltipContent.tsx - Inline styles using theme tokens
  • clients/fidesui/src/hoc/CustomStatistic.tsx - Size variants and font updates
  • clients/admin-ui/src/home/CommandBar.tsx - Use design tokens via antTheme.useToken(), Carbon icons via Icons.*
  • clients/admin-ui/src/home/DSRStatusCard.tsx - Remove Spin, add Card loading, memoize SLA data
  • clients/admin-ui/src/home/PostureCard.tsx - Remove Spin, use Card loading
  • clients/admin-ui/src/home/SystemCoverageCard.tsx - Move link to card extra, remove bottom button
  • clients/admin-ui/src/home/TrendCard.tsx - Use design tokens, Carbon icons via fidesui
  • clients/admin-ui/src/home/HomeDashboard.tsx - Simplify by removing briefing state management
  • clients/admin-ui/src/home/AgentBriefingBanner.tsx - Self-contained with own data fetching and dismissal

Steps to Confirm

  1. Run the admin-ui dev server (npm run dev in clients/admin-ui/)
  2. Navigate to the dashboard (home page with alphaDashboard flag enabled)
  3. Verify dashboard cards display correct theme colors (not default antd blue/green/red)
  4. Verify CustomCard titles appear at 14px and tabs at 12px
  5. Verify RadarChart shows concentric grid circles when no data is available
  6. Verify DSRStatusCard shows loading skeleton while data fetches

Pre-Merge Checklist

  • Issue requirements met
  • All CI pipelines succeeded
  • CHANGELOG.md updated
    • Add a db-migration This indicates that a change includes a database migration label to the entry if your change includes a DB migration
    • Add a high-risk This issue suggests changes that have a high-probability of breaking existing code label to the entry if your change includes a high-risk change (i.e. potential for performance impact or unexpected regression) that should be flagged
    • Updates unreleased work already in Changelog, no new entry necessary
  • UX feedback:
    • All UX related changes have been reviewed by a designer
    • No UX review needed
  • Followup issues:
    • Followup issues created
    • No followup issues
  • Database migrations:
    • No migrations
  • Documentation:
    • No documentation updates required

kruulik and others added 13 commits March 20, 2026 11:57
- Move briefing state management into AgentBriefingBanner component
- Re-add loading state to DSRStatusCard
- Move catmullRomClosedPath to chart-utils with division-by-zero guard
- Remove commented-out debug code from CommandBar and PostureCard
- Update CustomCard docstring to match current implementation

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@vercel
Copy link
Copy Markdown
Contributor

vercel bot commented Mar 25, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
fides-plus-nightly Ready Ready Preview, Comment Mar 26, 2026 9:45pm
1 Skipped Deployment
Project Deployment Actions Updated (UTC)
fides-privacy-center Ignored Ignored Mar 26, 2026 9:45pm

Request Review

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Unrelated files reverted: ColumnDropdown, _app.tsx, CustomCard.module.scss,
next-env.d.ts. Theme changes (index.ts, dark-theme.ts, default-theme.ts)
moved to fix/ant-theme-tokens branch (PR #7759).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
kruulik added 2 commits March 25, 2026 20:30
# Conflicts:
#	clients/admin-ui/src/home/DSRStatusCard.tsx
#	clients/admin-ui/src/home/HomeDashboard.tsx
#	clients/admin-ui/src/home/PostureCard.tsx
#	clients/admin-ui/src/home/PriorityActionsCard.tsx
#	clients/admin-ui/src/home/SystemCoverageCard.tsx
@kruulik
Copy link
Copy Markdown
Contributor Author

kruulik commented Mar 26, 2026

@claude

@kruulik kruulik requested review from galvana, gilluminate and lucanovera and removed request for gilluminate March 26, 2026 00:50
Copy link
Copy Markdown

@claude claude bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this is a clean, well-scoped polish pass. The shift to antTheme.useToken() tokens, replacing <Spin> with the Card loading prop, making AgentBriefingBanner self-contained, and the catmullRomClosedPath weight overlay are all good improvements. The RoundedBarCell SVG path logic is correct and handles the first/last segment edge cases well.

One behavioral regression worth discussing (see inline), and a handful of minor suggestions — nothing blocking.

Summary of findings:

  • Suggestion (AgentBriefingBanner, line 20): Dismissal is now in-component useState, dropping the sessionStorage persistence from the old HomeDashboard. The banner will reappear on any SPA navigation that remounts the component.
  • Nice to have (AgentBriefingBanner, line 11): SEVERITY_STYLE typed as Record<string, string> instead of the narrower Record<ActionSeverity, string>.
  • Suggestion (CommandBar, lines 49–55): valueColorMap is a plain object literal recreated on every render; useMemo would be consistent with the previous module-level constant approach.
  • Nice to have (PostureCard, line 115): fontSize: 48 is a magic number — a design token would be more maintainable.
  • Nice to have (DSRStatusCard, line 40): The silent exclusion of the update field from sla_health would benefit from a short inline comment.
  • Nice to have (RadarChart, lines 261–276): stroke* props on the weight <Radar> are dead code when a shape prop is provided.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 26, 2026

Greptile Summary

This PR delivers a broad set of dashboard UI polish changes: theme token adoption, font-size reductions via design tokens, loading skeleton improvements (replacing <Spin> wrappers with the Card loading prop), and a refactor to make AgentBriefingBanner self-contained. Most changes are clean and well-structured, but there are a couple of issues worth addressing before merging.

Key concerns:

  • Banner dismissal regression: AgentBriefingBanner now uses plain useState for dismissal visibility. The previous sessionStorage-backed persistence (in HomeDashboard) was not ported into the refactored component, so the banner will reappear every time the user navigates back to the dashboard within the same tab session.
  • Potential weight tooltip bug in PostureCard: The tooltip renders dim.weight * 100 using the raw API weight value, while the chart normalizes weights relative to the maximum (weight / maxWeight * 100). If the API returns integer-valued weights (common for relative rankings), the tooltip could display percentages like 300% or 500%.
  • Tab label font-size mismatch in CustomCard: The docstring and PR description both state that tab labels should render at token.fontSizeSM, but the implementation uses token.fontSize instead.
  • Variable naming in catmullRomClosedPath: Single/two-character names (p0, p1, d0, a1, etc.) throughout the new math-heavy function conflict with the project's full-name variable convention.
  • PR size: This PR touches 24 files, which exceeds the 15-file limit established in the project's review guidelines.

Confidence Score: 3/5

  • Needs targeted fixes before merge — two likely user-facing bugs and one documentation/code mismatch.
  • The banner dismissal regression (banner reappears on every navigation) and the potential weight tooltip percentage display error are both concrete, reproducible user-facing issues. The tab label font-size discrepancy between code and docstring is also a clear bug. These aren't theoretical edge cases — they affect normal usage flows of the dashboard.
  • clients/admin-ui/src/home/AgentBriefingBanner.tsx (dismissal regression), clients/admin-ui/src/home/PostureCard.tsx (weight tooltip), and clients/fidesui/src/hoc/CustomCard.tsx (tab label font size)

Important Files Changed

Filename Overview
clients/admin-ui/src/home/AgentBriefingBanner.tsx Made self-contained with own data fetching, but removes sessionStorage-based dismissal persistence (banner reappears on navigation) and hardcodes alert type to "info" regardless of severity
clients/admin-ui/src/home/PostureCard.tsx Removes Spin wrapper, adds weight rendering to radar chart, but the tooltip weight display uses dim.weight * 100 which may be incorrect depending on the API weight format
clients/fidesui/src/hoc/CustomCard.tsx Adds tabList font-size wrapping and changes title from Typography.Title level={5} to Typography.Text strong, but docstring says tab labels use fontSizeSM while the code uses token.fontSize
clients/fidesui/src/components/charts/chart-utils.ts Adds catmullRomClosedPath with epsilon guard for division-by-zero, but uses single/two-character variable names throughout (p0, p1, d0, a1, etc.) violating the naming convention rule
clients/admin-ui/src/home/CommandBar.tsx Good refactor: uses design tokens via antTheme.useToken(), Carbon icons via fidesui Icons, and adds max-width container
clients/admin-ui/src/home/DSRStatusCard.tsx Replaces Spin with Card loading prop, memoizes SLA data with update key excluded, improves overdue link from button to NextLink
clients/fidesui/src/components/charts/RadarChart.tsx Improved empty state using EMPTY_GRID_DATA (3-point grid), adds weight shape overlay via WeightShape component and catmullRomClosedPath

Comments Outside Diff (5)

  1. clients/admin-ui/src/home/AgentBriefingBanner.tsx, line 65-68 (link)

    P1 Banner dismissal no longer persists across navigation

    The refactored component uses useState(true) for visible, which means dismissal only lasts for the current component lifecycle. Previously, HomeDashboard stored the dismissed state in sessionStorage so the banner would stay hidden if the user navigated away and returned to the dashboard within the same tab session.

    With this change, every time the user navigates back to the dashboard, the banner will reappear — even if they already dismissed it.

    Consider restoring the sessionStorage persistence inside the banner itself, since it now owns its own dismissal:

  2. clients/admin-ui/src/home/AgentBriefingBanner.tsx, line 46-51 (link)

    P2 Alert type hardcoded to "info" — severity-based styling lost

    SEVERITY_STYLE is defined but only applied to the quick-action link CSS classes. The <Alert> component's type prop is now always "info", whereas before it was derived from the highest-severity action (critical/high → "error", medium → "warning"). This removes the visual urgency indicator from the banner itself when there are critical items.

    If this is an intentional design decision (e.g., always AI-branded style), a brief comment would help clarify that for future readers.

  3. clients/admin-ui/src/home/PostureCard.tsx, line 761-764 (link)

    P1 Weight tooltip may display incorrect percentage

    dim comes from posture.dimensions (raw API values), while the radar chart's weight prop is the normalized value (dimension.weight / maxWeight) * 100. The tooltip therefore computes dim.weight * 100 using the raw API weight.

    If the API returns weights as integers (e.g., 3, 5, 2 — a common relative-weight pattern), then dim.weight * 100 would produce values like 300%, 500%, 200%, which are clearly wrong. The intended display is the proportion of total weight for each dimension.

    Consider deriving the displayed percentage from the same normalization used for the chart, e.g.:

    const totalWeight = posture.dimensions.reduce((sum, d) => sum + d.weight, 0);
    // …in renderTooltip:
    {dim && totalWeight > 0 && (
      <span className="text-xs opacity-65">
        {Math.round((dim.weight / totalWeight) * 100)}% weight
      </span>
    )}
  4. clients/fidesui/src/hoc/CustomCard.tsx, line 1799-1806 (link)

    P2 Tab label font size uses token.fontSize but docstring says token.fontSizeSM

    The updated docstring (line 1837) states "Tab labels are wrapped at token.fontSizeSM", and the PR description also lists token.fontSizeSM as the intended size for tabs. However, the implementation wraps labels at token.fontSize (the default body size), not the smaller token.fontSizeSM.

  5. clients/fidesui/src/components/charts/chart-utils.ts, line 1736-1747 (link)

    P2 Short variable names throughout catmullRomClosedPath

    Variables like p0, p1, p2, p3, d0, d1, d2, a0, a1, a2, n, and eps are all 1–3 characters, which conflicts with the project convention of using full descriptive names for variables.

    Consider using names like prevPoint, currPoint, nextPoint, nextNextPoint, dist01, dist12, dist23, alpha01, alpha12, alpha23, pointCount, and epsilon to improve readability without ambiguity.

    Rule Used: Use full names for variables, not 1 to 2 character... (source)

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Reviews (1): Last reviewed commit: "more polish" | Re-trigger Greptile

- Narrow SEVERITY_STYLE type to Record<ActionSeverity, string>
- Derive valueColor from BAND_STATUS instead of duplicated map
- Remove dead stroke props from weight Radar (shape handles rendering)
- Fix CustomCard docstring (fontSizeSM → fontSize)
- Simplify DSRStatusCard SLA rendering

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@kruulik kruulik enabled auto-merge March 26, 2026 21:41
@kruulik kruulik added this pull request to the merge queue Mar 26, 2026
Merged via the queue into main with commit c95c643 Mar 26, 2026
47 of 50 checks passed
@kruulik kruulik deleted the dashboard-ui-polish branch March 26, 2026 21:59
galvana pushed a commit that referenced this pull request Mar 26, 2026
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants