Conversation
Introduces the dashboard time-series chart UI: - ChartCard component with line and checkerboard visualizations - ChartFilterModal for per-chart filter configuration - HostsEnrolledCard using the new chart infrastructure - Sun/Moon icons - charts API client and endpoint - Dashboard layout updates
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #43878 +/- ##
==========================================
- Coverage 66.91% 66.90% -0.02%
==========================================
Files 2600 2606 +6
Lines 208978 209323 +345
Branches 9342 9469 +127
==========================================
+ Hits 139838 140041 +203
- Misses 56398 56537 +139
- Partials 12742 12745 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughDashboardPage now renders two new cards—ChartCard and HostsEnrolledCard—in a new charts row and removes the previous host-summary section. ChartCard fetches time-series data via a new charts service endpoint and renders either CheckerboardViz (SVG grid) or LineChartViz (Recharts line chart), with a ChartFilterModal for labels/platforms/host include/exclude filters. HostsEnrolledCard shows platform enrollment counts as a vertical bar chart. New TypeScript types, SCSS styles, tests, and a 🚥 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 |
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (7)
frontend/pages/DashboardPage/cards/HostsEnrolledCard/HostsEnrolledCard.tsx (3)
85-89:<Cell>children with a uniform fill are redundant.Every
Celluses the sameBAR_COLOR, so settingfill={BAR_COLOR}directly on<Bar>produces the same result with less markup and avoids per-datum keys.Proposed simplification
- <Bar dataKey="count" radius={[0, 4, 4, 0]} barSize={16}> - {data.map((entry) => ( - <Cell key={entry.label} fill={BAR_COLOR} /> - ))} - </Bar> + <Bar dataKey="count" fill={BAR_COLOR} radius={[0, 4, 4, 0]} barSize={16} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/pages/DashboardPage/cards/HostsEnrolledCard/HostsEnrolledCard.tsx` around lines 85 - 89, The per-entry <Cell> elements are redundant because each cell uses the same BAR_COLOR; update the Bar component in HostsEnrolledCard (the <Bar dataKey="count" ...> block) to set fill={BAR_COLOR} directly on <Bar> and remove the mapped <Cell key={entry.label} fill={BAR_COLOR} /> elements (and their data.map usage), leaving the Bar with radius and barSize intact so rendering remains identical but with simpler markup.
37-43:formatTickcan emit awkward labels for large values.
10500 → "10.5k"and1234 → "1.2k"are fine, but values like12345become"12.3k"(lossy precision is acceptable), while999999becomes"999.9k"rather than"1M". If the dashboard is expected to display tens of thousands of hosts you may also want anMbranch and/or useIntl.NumberFormat('en', { notation: 'compact' })to handle locale/grouping consistently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/pages/DashboardPage/cards/HostsEnrolledCard/HostsEnrolledCard.tsx` around lines 37 - 43, The formatTick function currently only formats thousands and produces awkward labels for very large numbers; update formatTick to use a compact/locale-aware formatter or add an explicit millions branch. Specifically, modify formatTick (the function) to either (a) return Intl.NumberFormat('en', { notation: 'compact', maximumFractionDigits: 1 }).format(value) so k/M are handled automatically and consistently, or (b) add an additional conditional for value >= 1_000_000 to produce "xM" (with one decimal when needed) and keep the existing k logic for >= 1000; ensure small numbers still return the plain numeric string.
68-69: Multiple<CartesianGrid>elements for mixed styling — valid approachThe two elements intentionally create different styling per direction: line 68 renders dashed vertical gridlines only, and line 69 renders solid horizontal gridlines. This pattern is supported by Recharts to achieve mixed styling. If this is the desired visual output (dashed verticals, solid horizontals), the current implementation is correct. If you want to simplify to a uniform style, a single
<CartesianGrid strokeDasharray="3 3" />would dash both directions, or remove one line to omit that direction entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/pages/DashboardPage/cards/HostsEnrolledCard/HostsEnrolledCard.tsx` around lines 68 - 69, The two CartesianGrid elements (CartesianGrid with horizontal={false} strokeDasharray="3 3" and CartesianGrid with vertical={false}) are intentional to produce dashed vertical lines and solid horizontal lines; if that's the desired mixed styling, keep both as-is; otherwise simplify to a single CartesianGrid (e.g., CartesianGrid strokeDasharray="3 3" to dash both directions, or remove one of the CartesianGrid components to omit that direction entirely) — update the JSX around the CartesianGrid elements accordingly.frontend/pages/DashboardPage/cards/HostsEnrolledCard/_styles.scss (1)
5-5: Avoid!importantunless truly necessary.Prefer increasing specificity or removing the upstream
h2margin rather than!important, which makes future overrides harder.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/pages/DashboardPage/cards/HostsEnrolledCard/_styles.scss` at line 5, Remove the !important on the rule that sets "margin: 0 0 $pad-medium !important;" and either increase selector specificity (e.g., target the card's wrapper or use a more specific selector for the h2 inside HostsEnrolledCard) or adjust the upstream h2 margin where it’s defined so the intended spacing applies without !important; update the selector that contains the margin declaration (the HostsEnrolledCard h2/margin rule) to be more specific or change the upstream h2 style to use the desired margin value.frontend/pages/DashboardPage/cards/ChartCard/ChartFilterModal/ChartFilterModal.tsx (1)
89-117: Pagination refetches every previous page on each scroll.
perPage: pageCount * PAGE_SIZEwithpage: 0means scrolling to page 5 re-requests 100 rows instead of fetching the next 20. Also,hasMore = hosts.length === pageCount * PAGE_SIZEis fragile at exact boundaries (one extra fetch when the total count is a multiple ofPAGE_SIZE). Consider fetching page-by-page and accumulating results, or usinguseInfiniteQuery, and computinghasMorefrom a responsecount/totalwhen available.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/pages/DashboardPage/cards/ChartCard/ChartFilterModal/ChartFilterModal.tsx` around lines 89 - 117, The current fetch uses hostsAPI.loadHosts({ page: 0, perPage: pageCount * PAGE_SIZE }) which refetches all prior pages on each scroll; change to page-by-page fetching (or switch to useInfiniteQuery) so each request asks for perPage: PAGE_SIZE and page: pageIndex (derive pageIndex from pageCount or the infinite query's cursor), update the query key to include the page index (e.g., ["chartFilterHosts", searchQuery, pageIndex]) and accumulate results into a single hosts array (append new pages instead of replacing), and compute hasMore from the response total/count (hostsData.count or hostsData.total) rather than comparing hosts.length to pageCount * PAGE_SIZE; also update handleScroll to increment the page index (setPageCount or loadNextPage) only when hasMore and not isLoadingHosts.frontend/pages/DashboardPage/cards/ChartCard/ChartCard.tests.tsx (1)
82-104: RestoreResizeObserverduring test teardown.Line 86 installs a global mock, but
afterAlldoes not restore it. This can mask ResizeObserver behavior in later tests sharing the same Jest environment.Proposed cleanup
describe("ChartCard", () => { const origGetBCR = Element.prototype.getBoundingClientRect; + const origResizeObserver = global.ResizeObserver; beforeAll(() => { global.ResizeObserver = (MockResizeObserver as unknown) as typeof ResizeObserver; @@ afterAll(() => { Element.prototype.getBoundingClientRect = origGetBCR; + global.ResizeObserver = origResizeObserver; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/pages/DashboardPage/cards/ChartCard/ChartCard.tests.tsx` around lines 82 - 104, The test installs a mock ResizeObserver in beforeAll but doesn't restore it; capture the original global.ResizeObserver (e.g., const origResizeObserver = global.ResizeObserver) before assigning MockResizeObserver in the beforeAll and then restore it in afterAll by reassigning global.ResizeObserver = origResizeObserver; also keep restoring Element.prototype.getBoundingClientRect (origGetBCR) as already done to avoid leaking the mock across tests.frontend/pages/DashboardPage/cards/ChartCard/CheckerboardViz.tests.tsx (1)
61-84: Restore the globalResizeObservermock after the suite.Line 65 replaces
global.ResizeObserver, butafterAllonly restoresgetBoundingClientRect. Restore the original observer too so this suite does not leak DOM behavior into later tests.Proposed cleanup
describe("CheckerboardViz", () => { const origGetBCR = Element.prototype.getBoundingClientRect; + const origResizeObserver = global.ResizeObserver; beforeAll(() => { global.ResizeObserver = (MockResizeObserver as unknown) as typeof ResizeObserver; @@ afterAll(() => { Element.prototype.getBoundingClientRect = origGetBCR; + global.ResizeObserver = origResizeObserver; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/pages/DashboardPage/cards/ChartCard/CheckerboardViz.tests.tsx` around lines 61 - 84, Capture the original global.ResizeObserver before replacing it in the beforeAll (e.g., const origResizeObserver = global.ResizeObserver), then assign the mock as you do now; in afterAll restore both the DOM method and the observer by setting Element.prototype.getBoundingClientRect = origGetBCR and global.ResizeObserver = origResizeObserver so the ResizeObserver mock installed in CheckerboardViz tests is cleaned up.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/pages/DashboardPage/cards/ChartCard/ChartCard.tsx`:
- Around line 173-180: The settings Button (the component with class
`${baseClass}__settings-btn` that calls setShowFilterModal(true) and renders
<Icon name="settings" />) is icon-only and needs an accessible name; add an
explicit label by providing an aria-label (e.g., aria-label="Configure chart
filters") or include visually hidden text inside the Button while keeping the
Icon for sighted users so screen readers announce the purpose of the button.
- Around line 48-54: hasActiveFilters currently treats any non-empty
filters.selectedHosts as an active filter even when filters.hostFilterMode ===
"none"; update the predicate in hasActiveFilters to only count host selections
as active when filters.hostFilterMode is "include" or "exclude" (i.e.,
hostFilterMode !== "none"), so use filters.selectedHosts.length > 0 &&
filters.hostFilterMode !== "none" along with the existing checks; adjust the
implementation in the hasActiveFilters function (referencing IChartFilterState,
hostFilterMode, and selectedHosts) accordingly.
- Around line 92-123: The component is using isFetching to show the loading
Spinner which also becomes true during background refetches and will replace
already-rendered chart data; change the render condition to use isLoading
(initial load) instead of isFetching so stale chartData remains visible during
background refreshes, i.e., update the renderChart logic that checks isFetching
to check isLoading and ensure useQuery (in this file) still returns chartData
for rendering while refetching.
In `@frontend/pages/DashboardPage/cards/ChartCard/ChartFilterModal/_styles.scss`:
- Around line 75-82: The SCSS selectors &__results-loading, &__results-error,
&__results-empty don't match the JSX in ChartFilterModal.tsx which renders
`${baseClass}__results-status`, so the styles never apply; to fix, update the
selector block in _styles.scss to target `&__results-status` (or alternatively
update ChartFilterModal.tsx to emit the three distinct classes and add an error
state), ensuring the padding, text-align, font-size and color rules are applied
to the rendered `.ChartFilterModal__results-status` element.
In
`@frontend/pages/DashboardPage/cards/ChartCard/ChartFilterModal/ChartFilterModal.tsx`:
- Around line 63-65: The current initializer forces hostFilterMode to "exclude"
when filters.hostFilterMode === "none", which mutates stored state silently;
instead keep hostFilterMode initialized from filters.hostFilterMode (do not
coerce "none"), introduce a separate UI-only value (e.g., hostTabIndex or
hostFilterModeUI) to control which tab is shown, and when applying changes only
set the persisted hostFilterMode to the UI selection if selectedHosts.length > 0
(otherwise persist "none"); update uses of hostFilterMode in the component to
read from filters.hostFilterMode for persisted state and from hostFilterModeUI
(or hostTabIndex) for presentation, and use setHostFilterMode only when
committing a non-empty selection.
- Around line 187-235: The SearchField is currently uncontrolled (uses
defaultValue) so handleClear (and other programmatic resets) don't update the
visible input; update renderHostSearch to pass a controlled prop
(value={searchInput}) and ensure handleSearchChange updates searchInput
accordingly, or alternatively force a remount by giving SearchField a key
derived from searchInput when cleared; also fix the status class mismatch by
replacing the single __results-status node with distinct classes that match the
stylesheet (use __results-loading when isLoadingHosts, __results-empty when
hosts.length===0, and __results-error for errors) so the styles defined in
_styles.scss apply; check references: SearchField, handleClear,
handleSearchChange, renderHostSearch, isLoadingHosts, hosts, toggleHost,
removeHost.
In `@frontend/pages/DashboardPage/cards/ChartCard/CheckerboardViz.tsx`:
- Around line 209-223: Add keyboard accessibility for checkerboard cells by
implementing focus/blur handlers and accessible labeling: extend the existing
handleMouseEnter logic into a new handleFocus (or update handleMouseEnter to
accept FocusEvent) so when a cell receives focus you call setHoveredCell(cell),
compute tooltip position using (e.currentTarget as
HTMLElement).getBoundingClientRect() and containerRef.current, then
setTooltipPos as you do on hover; add a handleBlur (or reuse handleMouseLeave)
to call setHoveredCell(null). Ensure each cell element rendered in the
CheckerboardViz has tabIndex={0}, role (e.g., "button" or "gridcell"), and an
aria-label that describes the cell value so keyboard and assistive-tech users
get the same information as hover. Reference: handleMouseEnter,
handleMouseLeave, setHoveredCell, setTooltipPos, containerRef, cellW.
In `@frontend/pages/DashboardPage/cards/ChartCard/LineChartViz.tsx`:
- Line 58: The current tickInterval computation in LineChartViz.tsx forces
interval=1 even for small datasets, hiding ticks; change the logic so that when
data.length <= 8 you set tickInterval to 0 (show all ticks) and only compute a
positive skip for larger datasets—e.g. set tickInterval = data.length <= 8 ? 0 :
Math.max(1, Math.floor(data.length / 8))—and ensure the XAxis/interval prop uses
this tickInterval value.
In `@frontend/pages/DashboardPage/DashboardPage.tsx`:
- Around line 272-314: The totals query is treated as valid zero data when
hostSummaryTotals is missing, causing a zero-count flash; update the useQuery
call to destructure and expose isLoading and isError (e.g., const { data:
hostSummaryTotals, isLoading: totalsLoading, isError: totalsError } =
useQuery(...)), change totalCounts (the useMemo that builds IHostPlatformCounts)
to return undefined/null when hostSummaryTotals is not present instead of a
zeroed object, and then in the DashboardPage render path (where totalCounts is
consumed) show a spinner when totalsLoading and an error UI when totalsError
rather than rendering cards with zero values so that pending/failed states are
not shown as enrolled zeros.
- Around line 301-313: The reduce over hostSummaryTotals.platforms can overwrite
the pre-seeded linux aggregate (all_linux_count); update the reducer to skip
assigning when item.platform === 'linux' so the initial linux value from
base/all_linux_count is preserved. In the function using
hostSummaryTotals.platforms.reduce<IHostPlatformCounts> (and acc/item
variables), change the conditional to only set acc[item.platform as keyof
IHostPlatformCounts] when item.platform is not "linux" (or otherwise merge
counts without replacing the precomputed all_linux_count) so the existing
dashboard aggregate remains intact.
---
Nitpick comments:
In `@frontend/pages/DashboardPage/cards/ChartCard/ChartCard.tests.tsx`:
- Around line 82-104: The test installs a mock ResizeObserver in beforeAll but
doesn't restore it; capture the original global.ResizeObserver (e.g., const
origResizeObserver = global.ResizeObserver) before assigning MockResizeObserver
in the beforeAll and then restore it in afterAll by reassigning
global.ResizeObserver = origResizeObserver; also keep restoring
Element.prototype.getBoundingClientRect (origGetBCR) as already done to avoid
leaking the mock across tests.
In
`@frontend/pages/DashboardPage/cards/ChartCard/ChartFilterModal/ChartFilterModal.tsx`:
- Around line 89-117: The current fetch uses hostsAPI.loadHosts({ page: 0,
perPage: pageCount * PAGE_SIZE }) which refetches all prior pages on each
scroll; change to page-by-page fetching (or switch to useInfiniteQuery) so each
request asks for perPage: PAGE_SIZE and page: pageIndex (derive pageIndex from
pageCount or the infinite query's cursor), update the query key to include the
page index (e.g., ["chartFilterHosts", searchQuery, pageIndex]) and accumulate
results into a single hosts array (append new pages instead of replacing), and
compute hasMore from the response total/count (hostsData.count or
hostsData.total) rather than comparing hosts.length to pageCount * PAGE_SIZE;
also update handleScroll to increment the page index (setPageCount or
loadNextPage) only when hasMore and not isLoadingHosts.
In `@frontend/pages/DashboardPage/cards/ChartCard/CheckerboardViz.tests.tsx`:
- Around line 61-84: Capture the original global.ResizeObserver before replacing
it in the beforeAll (e.g., const origResizeObserver = global.ResizeObserver),
then assign the mock as you do now; in afterAll restore both the DOM method and
the observer by setting Element.prototype.getBoundingClientRect = origGetBCR and
global.ResizeObserver = origResizeObserver so the ResizeObserver mock installed
in CheckerboardViz tests is cleaned up.
In `@frontend/pages/DashboardPage/cards/HostsEnrolledCard/_styles.scss`:
- Line 5: Remove the !important on the rule that sets "margin: 0 0 $pad-medium
!important;" and either increase selector specificity (e.g., target the card's
wrapper or use a more specific selector for the h2 inside HostsEnrolledCard) or
adjust the upstream h2 margin where it’s defined so the intended spacing applies
without !important; update the selector that contains the margin declaration
(the HostsEnrolledCard h2/margin rule) to be more specific or change the
upstream h2 style to use the desired margin value.
In `@frontend/pages/DashboardPage/cards/HostsEnrolledCard/HostsEnrolledCard.tsx`:
- Around line 85-89: The per-entry <Cell> elements are redundant because each
cell uses the same BAR_COLOR; update the Bar component in HostsEnrolledCard (the
<Bar dataKey="count" ...> block) to set fill={BAR_COLOR} directly on <Bar> and
remove the mapped <Cell key={entry.label} fill={BAR_COLOR} /> elements (and
their data.map usage), leaving the Bar with radius and barSize intact so
rendering remains identical but with simpler markup.
- Around line 37-43: The formatTick function currently only formats thousands
and produces awkward labels for very large numbers; update formatTick to use a
compact/locale-aware formatter or add an explicit millions branch. Specifically,
modify formatTick (the function) to either (a) return Intl.NumberFormat('en', {
notation: 'compact', maximumFractionDigits: 1 }).format(value) so k/M are
handled automatically and consistently, or (b) add an additional conditional for
value >= 1_000_000 to produce "xM" (with one decimal when needed) and keep the
existing k logic for >= 1000; ensure small numbers still return the plain
numeric string.
- Around line 68-69: The two CartesianGrid elements (CartesianGrid with
horizontal={false} strokeDasharray="3 3" and CartesianGrid with
vertical={false}) are intentional to produce dashed vertical lines and solid
horizontal lines; if that's the desired mixed styling, keep both as-is;
otherwise simplify to a single CartesianGrid (e.g., CartesianGrid
strokeDasharray="3 3" to dash both directions, or remove one of the
CartesianGrid components to omit that direction entirely) — update the JSX
around the CartesianGrid elements accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 03e98df4-b4f5-4ce5-9e30-0215e451185f
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (21)
frontend/components/icons/Moon.tsxfrontend/components/icons/Sun.tsxfrontend/pages/DashboardPage/DashboardPage.tsxfrontend/pages/DashboardPage/_styles.scssfrontend/pages/DashboardPage/cards/ChartCard/ChartCard.tests.tsxfrontend/pages/DashboardPage/cards/ChartCard/ChartCard.tsxfrontend/pages/DashboardPage/cards/ChartCard/ChartFilterModal/ChartFilterModal.tsxfrontend/pages/DashboardPage/cards/ChartCard/ChartFilterModal/_styles.scssfrontend/pages/DashboardPage/cards/ChartCard/ChartFilterModal/index.tsfrontend/pages/DashboardPage/cards/ChartCard/CheckerboardViz.tests.tsxfrontend/pages/DashboardPage/cards/ChartCard/CheckerboardViz.tsxfrontend/pages/DashboardPage/cards/ChartCard/LineChartViz.tsxfrontend/pages/DashboardPage/cards/ChartCard/_styles.scssfrontend/pages/DashboardPage/cards/ChartCard/index.tsfrontend/pages/DashboardPage/cards/ChartCard/types.tsfrontend/pages/DashboardPage/cards/HostsEnrolledCard/HostsEnrolledCard.tsxfrontend/pages/DashboardPage/cards/HostsEnrolledCard/_styles.scssfrontend/pages/DashboardPage/cards/HostsEnrolledCard/index.tsfrontend/services/entities/charts.tsfrontend/utilities/endpoints.tspackage.json
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 similar comment
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/pages/DashboardPage/cards/ChartCard/CheckerboardViz.tsx (1)
48-52: DeadUSE_Y_AXIS_ICONStoggle leaves Sun/Moon imports and an entire render branch unreachable.With
USE_Y_AXIS_ICONS = falsehard-coded, the branch at lines 243-272, thepickIconSizehelper (229-234), theiconSizecalculation (235), and theSun/Moonimports (lines 4-5) — which this PR adds — are all unreachable. This shows up as dead code / unused-import lint noise and obscures which mode is actually shipping. Either drive the toggle from a prop (so consumers can opt in), or remove the icon branch + unused helpers/imports and reintroduce them when the feature is wired up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/pages/DashboardPage/cards/ChartCard/CheckerboardViz.tsx` around lines 48 - 52, The hard-coded USE_Y_AXIS_ICONS constant makes the icon codepath unreachable; either convert USE_Y_AXIS_ICONS into a prop on CheckerboardViz so consumers can enable Y-axis icons (update the component signature to accept useYAxisIcons and replace uses of USE_Y_AXIS_ICONS with that prop, ensuring pickIconSize, iconSize calculation, and Sun/Moon imports are used), or remove the unused icon logic entirely (delete the pickIconSize helper, the iconSize calculation, the render branch that renders Sun/Moon, and remove the Sun and Moon imports) to eliminate dead code and unused-import lint noise.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@frontend/pages/DashboardPage/cards/ChartCard/CheckerboardViz.tsx`:
- Around line 48-52: The hard-coded USE_Y_AXIS_ICONS constant makes the icon
codepath unreachable; either convert USE_Y_AXIS_ICONS into a prop on
CheckerboardViz so consumers can enable Y-axis icons (update the component
signature to accept useYAxisIcons and replace uses of USE_Y_AXIS_ICONS with that
prop, ensuring pickIconSize, iconSize calculation, and Sun/Moon imports are
used), or remove the unused icon logic entirely (delete the pickIconSize helper,
the iconSize calculation, the render branch that renders Sun/Moon, and remove
the Sun and Moon imports) to eliminate dead code and unused-import lint noise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7e5c3abe-847a-4600-ab9f-56467ec8dbbc
📒 Files selected for processing (5)
frontend/pages/DashboardPage/cards/ChartCard/ChartCard.tests.tsxfrontend/pages/DashboardPage/cards/ChartCard/ChartCard.tsxfrontend/pages/DashboardPage/cards/ChartCard/CheckerboardViz.tsxfrontend/pages/DashboardPage/cards/ChartCard/_styles.scssfrontend/pages/DashboardPage/cards/ChartCard/types.ts
✅ Files skipped from review due to trivial changes (2)
- frontend/pages/DashboardPage/cards/ChartCard/_styles.scss
- frontend/pages/DashboardPage/cards/ChartCard/ChartCard.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/pages/DashboardPage/cards/ChartCard/types.ts
- frontend/pages/DashboardPage/cards/ChartCard/ChartCard.tests.tsx
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
frontend/pages/DashboardPage/DashboardPage.tsx (1)
270-315:⚠️ Potential issue | 🟠 MajorDon’t render missing totals as zero enrolled hosts.
totalCountsstill falls back to all-zero counts when the totals query is pending, disabled, or failed, andHostsEnrolledCardrenders those values as real data. Keep the card in a loading/error state untilhostSummaryTotalsis available.🐛 Proposed fix
- const { data: hostSummaryTotals } = useQuery< + const { + data: hostSummaryTotals, + isFetching: isHostSummaryTotalsFetching, + error: errorHostSummaryTotals, + } = useQuery< IHostSummary, Error, IHostSummary >( @@ - const totalCounts = useMemo<IHostPlatformCounts>(() => { + const totalCounts = useMemo<IHostPlatformCounts | undefined>(() => { const base: IHostPlatformCounts = { darwin: 0, windows: 0, @@ }; if (!hostSummaryTotals?.platforms) { - return base; + return undefined; } @@ <div className={`${baseClass}__charts-row`}> <Card paddingSize="xlarge" borderRadiusSize="large"> - <HostsEnrolledCard counts={totalCounts} /> + {errorHostSummaryTotals ? ( + <DataError verticalPaddingSize="pad-large" /> + ) : isHostSummaryTotalsFetching && !hostSummaryTotals ? ( + <Spinner /> + ) : totalCounts ? ( + <HostsEnrolledCard counts={totalCounts} /> + ) : ( + <DataError verticalPaddingSize="pad-large" /> + )} </Card>Also applies to: 944-947
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/pages/DashboardPage/DashboardPage.tsx` around lines 270 - 315, The totalCounts memo currently returns an all-zero IHostPlatformCounts when hostSummaryTotals is missing, causing HostsEnrolledCard to show zeroes instead of a loading/error state; change totalCounts to return undefined (type IHostPlatformCounts | undefined) when hostSummaryTotals is not available (i.e., if !hostSummaryTotals or !hostSummaryTotals.platforms) and only compute concrete counts when hostSummaryTotals exists, then ensure the consumer (HostsEnrolledCard) receives this undefined value so it can render its loading/error UI rather than treating zeros as real data.frontend/pages/DashboardPage/cards/ChartCard/CheckerboardViz.tsx (1)
211-225:⚠️ Potential issue | 🟠 MajorMake checkerboard cell details reachable by keyboard.
The cells now have labels, but the tooltip is still mouse-only. Add focus/blur handlers and make each rect focusable so keyboard users can inspect the same values.
♿ Proposed fix
- const handleMouseEnter = (cell: ICellData, e: React.MouseEvent) => { + const showTooltipForCell = (cell: ICellData, target: SVGElement) => { setHoveredCell(cell); - const rect = (e.target as SVGElement).getBoundingClientRect(); + const rect = target.getBoundingClientRect(); const containerRect = containerRef.current?.getBoundingClientRect(); if (containerRect) { setTooltipPos({ x: rect.left - containerRect.left + cellW / 2, y: rect.top - containerRect.top - 8, }); } }; + + const handleMouseEnter = ( + cell: ICellData, + e: React.MouseEvent<SVGElement> + ) => { + showTooltipForCell(cell, e.currentTarget); + }; + + const handleFocus = (cell: ICellData, e: React.FocusEvent<SVGElement>) => { + showTooltipForCell(cell, e.currentTarget); + }; const handleMouseLeave = () => { setHoveredCell(null); }; @@ role="img" + tabIndex={0} aria-label={`${cell.dayLabel}, ${cell.hourLabel}: ${cell.percentage}% of hosts online`} onMouseEnter={(e) => handleMouseEnter(cell, e)} onMouseLeave={handleMouseLeave} + onFocus={(e) => handleFocus(cell, e)} + onBlur={handleMouseLeave} />Also applies to: 317-320
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/pages/DashboardPage/cards/ChartCard/CheckerboardViz.tsx` around lines 211 - 225, Add keyboard accessibility by making each SVG rect focusable (set tabIndex=0 on the rect elements) and adding focus/blur handlers that mirror the mouse handlers; implement onFocus handler (e.g., handleFocus) that accepts a React.FocusEvent, calls setHoveredCell with the cell, computes tooltip position using (e.currentTarget as SVGElement).getBoundingClientRect() and containerRef.current?.getBoundingClientRect(), then calls setTooltipPos with the same x/y logic as handleMouseEnter, and implement onBlur to call setHoveredCell(null) (mirroring handleMouseLeave). Update the rect JSX to include tabIndex={0}, onFocus={handleFocus}, and onBlur={handleBlur}; apply the same changes for the duplicate block referenced around lines 317-320.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/pages/DashboardPage/cards/ChartCard/ChartCard.tsx`:
- Around line 64-107: The ChartCard currently omits team scoping: confirm the
backend charts endpoint exists and the exact team parameter name (e.g., team_id
or team_id_for_api) in server/api_endpoints/api_endpoints.yml and the handler;
then add that field to the frontend IChartRequestParams in
frontend/services/entities/charts.ts; modify ChartCard to accept a teamIdForApi
prop (or the exact name agreed with backend) and include it in the queryParams
object and in the useQuery key so the cache is scoped by team, and pass the same
prop from DashboardPage when rendering <ChartCard />; finally ensure
chartsAPI.getChartData is called with the team parameter included.
---
Duplicate comments:
In `@frontend/pages/DashboardPage/cards/ChartCard/CheckerboardViz.tsx`:
- Around line 211-225: Add keyboard accessibility by making each SVG rect
focusable (set tabIndex=0 on the rect elements) and adding focus/blur handlers
that mirror the mouse handlers; implement onFocus handler (e.g., handleFocus)
that accepts a React.FocusEvent, calls setHoveredCell with the cell, computes
tooltip position using (e.currentTarget as SVGElement).getBoundingClientRect()
and containerRef.current?.getBoundingClientRect(), then calls setTooltipPos with
the same x/y logic as handleMouseEnter, and implement onBlur to call
setHoveredCell(null) (mirroring handleMouseLeave). Update the rect JSX to
include tabIndex={0}, onFocus={handleFocus}, and onBlur={handleBlur}; apply the
same changes for the duplicate block referenced around lines 317-320.
In `@frontend/pages/DashboardPage/DashboardPage.tsx`:
- Around line 270-315: The totalCounts memo currently returns an all-zero
IHostPlatformCounts when hostSummaryTotals is missing, causing HostsEnrolledCard
to show zeroes instead of a loading/error state; change totalCounts to return
undefined (type IHostPlatformCounts | undefined) when hostSummaryTotals is not
available (i.e., if !hostSummaryTotals or !hostSummaryTotals.platforms) and only
compute concrete counts when hostSummaryTotals exists, then ensure the consumer
(HostsEnrolledCard) receives this undefined value so it can render its
loading/error UI rather than treating zeros as real data.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 44252a39-c4cc-4f9d-abc5-86a5bde5489c
📒 Files selected for processing (5)
frontend/pages/DashboardPage/DashboardPage.tsxfrontend/pages/DashboardPage/cards/ChartCard/ChartCard.tsxfrontend/pages/DashboardPage/cards/ChartCard/ChartFilterModal/ChartFilterModal.tsxfrontend/pages/DashboardPage/cards/ChartCard/ChartFilterModal/_styles.scssfrontend/pages/DashboardPage/cards/ChartCard/CheckerboardViz.tsx
✅ Files skipped from review due to trivial changes (1)
- frontend/pages/DashboardPage/cards/ChartCard/ChartFilterModal/_styles.scss
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/pages/DashboardPage/cards/ChartCard/ChartFilterModal/ChartFilterModal.tsx
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
There was a problem hiding this comment.
Pull request overview
Adds new dashboard chart widgets that consume the new charts API, including a reusable chart card with filtering and two initial visualizations.
Changes:
- Added Recharts dependency and a new
/charts/:metricfrontend service. - Introduced
ChartCard(with checkerboard + line visualizations) andChartFilterModalfor label/platform/host filtering. - Added a new “Hosts enrolled” bar chart card and placed both charts into a new dashboard grid row.
Reviewed changes
Copilot reviewed 19 out of 21 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| yarn.lock | Locks new frontend dependencies pulled in by Recharts. |
| package.json | Adds recharts dependency. |
| frontend/utilities/endpoints.ts | Adds charts endpoint helper. |
| frontend/services/entities/charts.ts | New charts API client + response/request types. |
| frontend/pages/DashboardPage/cards/HostsEnrolledCard/* | New “Hosts enrolled” bar chart component + styles/exports. |
| frontend/pages/DashboardPage/cards/ChartCard/* | New chart card, visualizations, filter modal, styles, and tests. |
| frontend/pages/DashboardPage/_styles.scss | Adds layout styling for the charts row. |
| frontend/pages/DashboardPage/DashboardPage.tsx | Renders the new chart cards and adds host-summary query for platform totals. |
| changes/43769-added-charts-to-dashboard | Release note entry for the new dashboard charts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@frontend/pages/DashboardPage/cards/ChartCard/ChartFilterModal/ChartFilterModal.tsx`:
- Around line 97-113: The current useQuery call for hosts (useQuery([...], () =>
hostsAPI.loadHosts(...))) only reads data and isLoading, which treats failures
as empty results; update the hook to also destructure isError and error (e.g.,
const { data: hostsData, isLoading: isLoadingHosts, isError: isHostsError,
error: hostsError } = useQuery(...)), and then change the UI logic that maps or
shows "No matching hosts" to first render an error state when isHostsError is
true (display hostsError.message or a friendly error component) before falling
back to empty-array/no-results behavior; apply the same change to the other
similar useQuery instance referenced around lines 236-243.
- Around line 43-56: The modal's host search is unscoped because
ChartFilterModal doesn't accept or pass the current team/fleet id; update the
ChartFilterModal signature to accept a currentTeamId prop (from ChartCard),
include currentTeamId in any React Query queryKey used for host loading (so
queries are scoped), and pass currentTeamId into the loadHosts call where hosts
are fetched; finally, update ChartCard to pass its currentTeamId down to
ChartFilterModal so the modal search is limited to the selected team/fleet.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d3beb22f-3904-4b73-92c0-80fe49bb2ef9
📒 Files selected for processing (7)
changes/43769-added-charts-to-dashboardfrontend/pages/DashboardPage/DashboardPage.tsxfrontend/pages/DashboardPage/cards/ChartCard/ChartCard.tsxfrontend/pages/DashboardPage/cards/ChartCard/ChartFilterModal/ChartFilterModal.tsxfrontend/pages/DashboardPage/cards/ChartCard/CheckerboardViz.tsxfrontend/pages/DashboardPage/cards/ChartCard/_styles.scssfrontend/services/entities/charts.ts
✅ Files skipped from review due to trivial changes (3)
- changes/43769-added-charts-to-dashboard
- frontend/pages/DashboardPage/cards/ChartCard/_styles.scss
- frontend/services/entities/charts.ts
|
@sgress454 Just let me know when bot review is done. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/pages/DashboardPage/cards/ChartCard/ChartCard.tsx`:
- Around line 67-101: The ChartCard's local chartFilters state persists across
team changes causing stale filters to be sent with a new fleet_id; add a
useEffect that watches currentTeamId and calls setChartFilters to reset
chartFilters to the initial IChartFilterState (labelIDs: [], platforms: [],
hostFilterMode: "none", selectedHosts: []) whenever currentTeamId changes so
filters are cleared for the new team; update the effect to reference
chartFilters only for resetting if needed and ensure it runs on currentTeamId
change.
In
`@frontend/pages/DashboardPage/cards/ChartCard/ChartFilterModal/ChartFilterModal.tsx`:
- Around line 140-147: The labels query currently loads global labels which can
include other teams; update the useQuery call that assigns `labels` so it is
keyed and parameterized by `currentTeamId` and calls `labelsAPI.summary` with
the team id (e.g. `labelsAPI.summary(currentTeamId)` or `{ team_id:
currentTeamId }` depending on API shape). Specifically, change the query key
from ["labelsSummary"] to ["labelsSummary", currentTeamId] and pass
`currentTeamId` into `labelsAPI.summary` so the returned `labels` are scoped to
the selected team (keep `DEFAULT_USE_QUERY_OPTIONS` and `staleTime` unchanged).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 90a1e07f-7a1a-4c6a-8aaf-9c1f28071ce3
📒 Files selected for processing (5)
frontend/pages/DashboardPage/cards/ChartCard/ChartCard.tests.tsxfrontend/pages/DashboardPage/cards/ChartCard/ChartCard.tsxfrontend/pages/DashboardPage/cards/ChartCard/ChartFilterModal/ChartFilterModal.tsxfrontend/pages/DashboardPage/cards/ChartCard/CheckerboardViz.tests.tsxfrontend/pages/DashboardPage/cards/ChartCard/types.ts
✅ Files skipped from review due to trivial changes (2)
- frontend/pages/DashboardPage/cards/ChartCard/types.ts
- frontend/pages/DashboardPage/cards/ChartCard/CheckerboardViz.tests.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/pages/DashboardPage/cards/ChartCard/ChartCard.tests.tsx
|
@lukeheath good to go! |
<!-- Add the related story/sub-task/bug number, like Resolves #123, or remove if NA --> **Related issue:** For #42812 # Details This PR implements a new bounded context, `chart`, with a single endpoint `/charts`. The context encompasses a framework for recording and querying and aggregating historical data for Fleet hosts, and returning that data via the API for the purpose of charting. This initial iteration has a full implementation of a dataset called "uptime" which captures which hosts were online hour-by-hour (online meaning, having been "seen" at some point during that hour). It has a partial implementation of a "cve" dataset which will capture which hosts were vulnerable to which CVEs during a given day. ### Data storage Data is stored in an SCD (slowly-changing dimension) format in the `host_scd_data` table, where the main "value" in a row is stored in the `host_bitmap` column, which is a `mediumblob` where each bit encodes a host ID (bit one represents host ID 1, bit 1444 represents host ID 1444, etc.). The set of bits set on a row represents that hosts for which that dataset is "on" during a given time period represented by the `valid_from` (inclusive) and `valid_to` (exclusive) dates, where a `valid_to` can have the special "sentinel" value 9999-12-31T00:00:00.000 meaning that the row is still "open" (the value represents everything from `valid_from` to the present). Additionally an `entity_id` column can be used for datasets with multiple dimensions, e.g. CVE exposure or software usage which would have entity IDs representing CVEs or software items respectively. ### Data collection Data is collected via a cron job that runs every 10 minutes. Each dataset has its own `Collect` method which will sample the data for the given moment. For example the "uptime" dataset gathers the set of hosts that are online at the moment, and the "cve" dataset will gather the set of hosts that are vulnerable to each CVE at that moment. The sample can then be recorded using one of two strategies: * `accumulate`: bitwise OR the sample with any data already recorded for the current hour, or add a new pre-closed row for that hour. * `snapshot`: if there is no open row, create one with the sample and `valid_to set` to the sentinel. Otherwise: * If the sample has the same value as the current open row, do nothing * If the sample has a different value and the current open row's `valid_from` is within the same hour, update the current row's value * If the sample has a different value and the current open row's `valid_from` is not within the same hour, close the current open row and start a new one with `valid_from` = the start of the current hour ### Data retrieval 1. Gets the set of host IDs to retrieve data for. This starts with the set of host IDs in the requested fleet (or all the hosts a user has access to if no `fleet_id` param was passed to the `/charts` endpoint), and further whittled down by any filter options supplied with the request (labels, platforms, etc.). 2. Finds all `host_scd_data` rows for the requested dataset and date range (i.e. all rows whose `valid_from` is < the date range end and `valid_to` is > the date range start). 3. Calculates the date ranges of the "buckets" to return datapoints for. For the uptime chart we default to 3-hour buckets, so we want 8 buckets per day. 4. Iterates over each bucket and finds the row or rows from host_scd_data that cover that bucket range. For datasets using the "accumulate" strategy, the values for those rows are ORed together. For "snapshot"s, we take the one active at the bucket end time to represent the bucket (e.g. "which hosts had a given CVE at the end of the day") ### Tools This PR includes two dev tools that don't require deep review: * **chart-backfill** - used to backfill data to various datasets for testing * **charts-collect** - used to collect data from a live server via the API and put into a local hosts_scd_data table # Checklist for submitter If some of the following don't apply, delete the relevant line. - [X] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. See [Changes files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/guides/committing-changes.md#changes-files) for more information. - [X] Input data is properly validated, `SELECT *` is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters. ## Testing - [X] Added/updated automated tests - [X] Where appropriate, [automated tests simulate multiple hosts and test for host isolation](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/reference/patterns-backend.md#unit-testing) (updates to one hosts's records do not affect another) - [X] QA'd all new/changed functionality manually - With [front-end branch](#43878) <img width="712" height="434" alt="image" src="https://github.com/user-attachments/assets/b2ccce49-b5fd-4076-b47f-0eea6a53260c" /> ## Database migrations - [X] Checked schema for all modified table for columns that will auto-update timestamps during migration. - [X] Confirmed that updating the timestamps is acceptable, and will not cause unwanted side effects. - [X] Ensured the correct collation is explicitly set for character columns (`COLLATE utf8mb4_unicode_ci`). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added charting bounded context: HTTP API for metrics (uptime, CVE), dataset registry, hosted dataset collection, background collection/cleanup with opt-out env. * New utilities: host bitmap operations and string-list/uint-list parsers. * New CLI tools to collect and backfill chart data. * **Database** * Migration and schema to store host time-series SCD chart data. * **Tests** * Extensive unit and integration tests for service, storage, caching, cron, and utilities. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
| ); | ||
| }; | ||
|
|
||
| export default CheckerboardViz; |
There was a problem hiding this comment.
Good stuff. I figured the checkerboard was a frontend dep.
There was a problem hiding this comment.
I tried using the recharts "scatter" chart but getting it to look right was more trouble than it was worth for something that's just a buncha boxes.
| () => chartsAPI.getChartData(selectedMetric, queryParams), | ||
| { | ||
| ...DEFAULT_USE_QUERY_OPTIONS, | ||
| staleTime: 300000, // 5 minutes |
|
|
||
| const renderChart = () => { | ||
| if (isLoading) { | ||
| return <Spinner includeContainer={false} verticalPadding="small" />; |
There was a problem hiding this comment.
FYI On main but not this branch, there's a new delay prop that will default to 250ms, which is what we want here since it's an API call. If it was indeterminate we'd want to pass delay=0.
There was a problem hiding this comment.
ok so no action required here?
There was a problem hiding this comment.
Correct no action here, just FYI that <Spinner> now defaults to a 250ms delay unless otherwise specified (which is what you want here.)
lukeheath
left a comment
There was a problem hiding this comment.
Nice work! Looks good. Some nits but nothing blocking.
| const [hoveredCell, setHoveredCell] = useState<ICellData | null>(null); | ||
| const [tooltipPos, setTooltipPos] = useState({ x: 0, y: 0 }); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
I believe this will run after the initial paint, which means there could a flash where it renders once with isWide = false and then repaints if it's updated to isWide = true. It may not be visible, in which case I don't think it's blocking.
99% of the time you want useEffect, but if you need to do DOM measurements there is useLayoutEffect, which runs after dom calculations but before the first paint.
There was a problem hiding this comment.
I think this is worth addressing but there's plenty more front-end PRs to come 😏
| }; | ||
| }, [dayLabels]); | ||
|
|
||
| const handleMouseEnter = (cell: ICellData, e: React.MouseEvent) => { |
There was a problem hiding this comment.
Nit that this will only work with a mouse, not on a touch device or tabbing with the keyboard. I think it's fine for POC, though.
There was a problem hiding this comment.
Good call though, for touch device i guess we can do this on touch?
Related issue: For #43769
Details
This PR adds "Hosts active" and "Hosts enrolled" charts to the dashboard.
New components:
/chartsendpoint created in Dashboard charts backend #43910Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Testing
Summary by CodeRabbit