Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
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.
| const [macCount, setMacCount] = useState(0); | ||
| const [windowsCount, setWindowsCount] = useState(0); | ||
| const [linuxCount, setLinuxCount] = useState(0); | ||
| const [chromeCount, setChromeCount] = useState(0); | ||
| const [iosCount, setIosCount] = useState(0); | ||
| const [ipadosCount, setIpadosCount] = useState(0); | ||
| const [androidCount, setAndroidCount] = useState(0); |
There was a problem hiding this comment.
This and the rest of the large deletions are just from removing the PlatformHostCounts element (the cards that showed the # of mac, windows and linux hosts). They should have been removed previously.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #44329 +/- ##
========================================
Coverage 66.78% 66.78%
========================================
Files 2630 2630
Lines 211232 211226 -6
Branches 9510 9387 -123
========================================
- Hits 141063 141062 -1
+ Misses 57348 57343 -5
Partials 12821 12821
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:
|
| <HostsEnrolledCard | ||
| counts={totalCounts} | ||
| builtInLabels={labels} | ||
| currentTeamId={teamIdForApi} | ||
| router={router} |
There was a problem hiding this comment.
adding new props to allow clicking on elements of the hosts enrolled chart to go to hosts list page
| onLabelClick: (index: number) => void; | ||
| } | ||
|
|
||
| const ClickableYAxisTick = ({ |
There was a problem hiding this comment.
new element to use for the y-axis ticks, to make them clickable when there's data.
WalkthroughThe PR simplifies dashboard host-count rendering by removing per-platform host count state and the Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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.
🧹 Nitpick comments (1)
frontend/pages/DashboardPage/cards/HostsEnrolledCard/_styles.scss (1)
22-32: Consider preserving focus indicators for keyboard accessibility.Suppressing all focus outlines with
outline: none !importantremoves visual feedback for keyboard users navigating the chart. While this addresses the "undesirable focus rectangles" bug, completely removing focus indicators can create accessibility barriers.Consider providing a custom focus style (e.g., a subtle box-shadow or border) instead of completely suppressing the outline, or ensure these chart elements are not focusable via keyboard (
tabindex="-1") if they're only intended for mouse interaction.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 95a144d6-5a89-496c-a3eb-669db123c9b7
📒 Files selected for processing (3)
frontend/pages/DashboardPage/DashboardPage.tsxfrontend/pages/DashboardPage/cards/HostsEnrolledCard/HostsEnrolledCard.tsxfrontend/pages/DashboardPage/cards/HostsEnrolledCard/_styles.scss
| <Tooltip | ||
| content={<HostsEnrolledTooltip />} | ||
| cursor={false} | ||
| isAnimationActive={false} |
There was a problem hiding this comment.
without this the tooltip "swoops" in from the side in a very weird manner
|
Added one more thing: hover on the hosts enrolled bars to see the actual counts. We lost this info when we removed the per-platform host count cards. |
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/HostsEnrolledCard/HostsEnrolledCard.tsx`:
- Around line 167-170: The bar rendering/interaction logic must use the same
guard as isTickClickable to avoid showing clickable affordances when
builtInLabels is missing; update the bar hover/cursor and onClick checks (the
code that currently only checks datum.count > 0 around the bar render and the
code path that calls navigateToPlatform) to instead call isTickClickable(index)
or replicate its logic (ensure datum exists, datum.count is truthy, and
getLabelId(datum.platform) !== undefined) so bars only show pointer/hover and
invoke navigateToPlatform when a resolved label id exists; apply the same change
to the other rendering block around navigateToPlatform referenced in the comment
(the block around lines 210-224).
- Around line 100-126: The ClickableYAxisTick component only handles pointer
clicks; make it keyboard-activatable by adding focus and keyboard handlers: when
isClickable(payload.index) is true, render the outer <g> with tabIndex={0},
role="link" (or "button" if semantics prefer), and an onKeyDown handler that
calls onLabelClick(payload.index) when Enter or Space is pressed; also set
aria-disabled or omit role/tabIndex when not clickable and ensure the clickable
className remains for focus styling so keyboard users can see focus state.
🪄 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: d5f64650-2697-4753-b334-da477738c8cc
📒 Files selected for processing (2)
frontend/pages/DashboardPage/cards/HostsEnrolledCard/HostsEnrolledCard.tsxfrontend/pages/DashboardPage/cards/HostsEnrolledCard/_styles.scss
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/pages/DashboardPage/cards/HostsEnrolledCard/_styles.scss
| const ClickableYAxisTick = ({ | ||
| x = 0, | ||
| y = 0, | ||
| payload, | ||
| isClickable, | ||
| onLabelClick, | ||
| }: IYAxisTickProps): JSX.Element => { | ||
| if (!payload) return <g />; | ||
| const clickable = isClickable(payload.index); | ||
| return ( | ||
| <g | ||
| transform={`translate(${x},${y})`} | ||
| onClick={clickable ? () => onLabelClick(payload.index) : undefined} | ||
| > | ||
| <text | ||
| x={0} | ||
| y={0} | ||
| dy={4} | ||
| textAnchor="end" | ||
| fontSize={14} | ||
| className={clickable ? `${baseClass}__tick--clickable` : undefined} | ||
| > | ||
| {payload.value} | ||
| </text> | ||
| </g> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Make the custom Y-axis tick keyboard-activatable.
This adds a new interactive control, but it only handles pointer clicks. Keyboard users cannot focus or activate these platform links.
Suggested fix
const ClickableYAxisTick = ({
x = 0,
y = 0,
payload,
isClickable,
onLabelClick,
}: IYAxisTickProps): JSX.Element => {
if (!payload) return <g />;
const clickable = isClickable(payload.index);
+ const handleActivate = () => {
+ if (clickable) onLabelClick(payload.index);
+ };
+
return (
<g
transform={`translate(${x},${y})`}
- onClick={clickable ? () => onLabelClick(payload.index) : undefined}
+ onClick={clickable ? handleActivate : undefined}
+ onKeyDown={
+ clickable
+ ? (e) => {
+ if (e.key === "Enter" || e.key === " ") {
+ e.preventDefault();
+ handleActivate();
+ }
+ }
+ : undefined
+ }
+ tabIndex={clickable ? 0 : -1}
+ role={clickable ? "button" : undefined}
+ aria-label={clickable ? `View ${payload.value} hosts` : undefined}
>
<text
x={0}
y={0}
dy={4}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const ClickableYAxisTick = ({ | |
| x = 0, | |
| y = 0, | |
| payload, | |
| isClickable, | |
| onLabelClick, | |
| }: IYAxisTickProps): JSX.Element => { | |
| if (!payload) return <g />; | |
| const clickable = isClickable(payload.index); | |
| return ( | |
| <g | |
| transform={`translate(${x},${y})`} | |
| onClick={clickable ? () => onLabelClick(payload.index) : undefined} | |
| > | |
| <text | |
| x={0} | |
| y={0} | |
| dy={4} | |
| textAnchor="end" | |
| fontSize={14} | |
| className={clickable ? `${baseClass}__tick--clickable` : undefined} | |
| > | |
| {payload.value} | |
| </text> | |
| </g> | |
| ); | |
| }; | |
| const ClickableYAxisTick = ({ | |
| x = 0, | |
| y = 0, | |
| payload, | |
| isClickable, | |
| onLabelClick, | |
| }: IYAxisTickProps): JSX.Element => { | |
| if (!payload) return <g />; | |
| const clickable = isClickable(payload.index); | |
| const handleActivate = () => { | |
| if (clickable) onLabelClick(payload.index); | |
| }; | |
| return ( | |
| <g | |
| transform={`translate(${x},${y})`} | |
| onClick={clickable ? handleActivate : undefined} | |
| onKeyDown={ | |
| clickable | |
| ? (e) => { | |
| if (e.key === "Enter" || e.key === " ") { | |
| e.preventDefault(); | |
| handleActivate(); | |
| } | |
| } | |
| : undefined | |
| } | |
| tabIndex={clickable ? 0 : -1} | |
| role={clickable ? "button" : undefined} | |
| aria-label={clickable ? `View ${payload.value} hosts` : undefined} | |
| > | |
| <text | |
| x={0} | |
| y={0} | |
| dy={4} | |
| textAnchor="end" | |
| fontSize={14} | |
| className={clickable ? `${baseClass}__tick--clickable` : undefined} | |
| > | |
| {payload.value} | |
| </text> | |
| </g> | |
| ); | |
| }; |
🤖 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 100 - 126, The ClickableYAxisTick component only handles pointer
clicks; make it keyboard-activatable by adding focus and keyboard handlers: when
isClickable(payload.index) is true, render the outer <g> with tabIndex={0},
role="link" (or "button" if semantics prefer), and an onKeyDown handler that
calls onLabelClick(payload.index) when Enter or Space is pressed; also set
aria-disabled or omit role/tabIndex when not clickable and ensure the clickable
className remains for focus styling so keyboard users can see focus state.
| const isTickClickable = (index: number) => { | ||
| const datum = data[index]; | ||
| if (!datum || !datum.count) return false; | ||
| return getLabelId(datum.platform) !== undefined; |
There was a problem hiding this comment.
Align bar click affordances with the actual navigation guard.
isTickClickable correctly requires both a non-zero count and a resolved label id, but the bars only check count > 0. Since builtInLabels is optional, a bar can show pointer/hover affordances and still no-op in navigateToPlatform.
Suggested fix
+ const isBarClickable = ({ platform, count }: IPlatformDatum) => {
+ return count > 0 && getLabelId(platform) !== undefined;
+ };
+
const isTickClickable = (index: number) => {
const datum = data[index];
if (!datum || !datum.count) return false;
return getLabelId(datum.platform) !== undefined;
};
@@
<Bar
dataKey="count"
radius={[0, 4, 4, 0]}
barSize={16}
isAnimationActive={false}
activeBar={{ fill: BAR_HOVER_COLOR }}
- onClick={(d) => handleBarClick(d.payload as IPlatformDatum)}
+ onClick={(d) => {
+ const datum = d.payload as IPlatformDatum;
+ if (isBarClickable(datum)) handleBarClick(datum);
+ }}
>
{data.map((entry) => (
<Cell
key={entry.label}
fill={BAR_COLOR}
className={
- entry.count > 0 ? `${baseClass}__bar--clickable` : undefined
+ isBarClickable(entry)
+ ? `${baseClass}__bar--clickable`
+ : undefined
}
/>
))}Also applies to: 210-224
🤖 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 167 - 170, The bar rendering/interaction logic must use the same
guard as isTickClickable to avoid showing clickable affordances when
builtInLabels is missing; update the bar hover/cursor and onClick checks (the
code that currently only checks datum.count > 0 around the bar render and the
code path that calls navigateToPlatform) to instead call isTickClickable(index)
or replicate its logic (ensure datum exists, datum.count is truthy, and
getLabelId(datum.platform) !== undefined) so bars only show pointer/hover and
invoke navigateToPlatform when a resolved label id exists; apply the same change
to the other rendering block around navigateToPlatform referenced in the comment
(the block around lines 210-224).
Cherry-pick of #44329 into the RC branch.
3 participants
Related issue: Resolves #44249
Checklist 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.
n/a, unreleased
Testing
Added/updated automated tests
not worth it for style fixes and accidental content removal
Where appropriate, automated tests simulate multiple hosts and test for host isolation (updates to one hosts's records do not affect another)
QA'd all new/changed functionality manually
For unreleased bug fixes in a release candidate, one of:
Summary by CodeRabbit
New Features
UI Improvements