Fleet UI: Empty state followups#45039
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #45039 +/- ##
=======================================
Coverage 66.77% 66.78%
=======================================
Files 2718 2718
Lines 218818 218806 -12
Branches 10748 10744 -4
=======================================
+ Hits 146123 146136 +13
+ Misses 59532 59506 -26
- Partials 13163 13164 +1
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:
|
RachelElysia
changed the title
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, low-risk frontend cleanup with new test coverage.
Extended reasoning...
Overview
This PR is a frontend-only follow-up to the empty-state work in #35483. It touches three host-related UI areas:
ManageHostsPage.tsx: removes a duplicate/now-unreachable empty-state branch and replaces a hardcoded"5,000"withMAX_SCRIPT_BATCH_TARGETS.toLocaleString().SelectReportModal: renames the CSSbaseClassfromselect-query-modaltoselect-report-modal(matches the SCSS file and component name), plus trivial whitespace.HostPolicies.tsx: collapses the conditional "policies.length === 0" branch into theTableContainer'semptyComponentprop, and drops the now-unneededdisplay: nonerule on.table-container__header.
Security risks
None. Frontend presentational changes with no new data flow, no auth/permissions logic, no external inputs, and no API surface.
Level of scrutiny
Low. The diff is mostly mechanical: dead-code removal, a constant substitution, a CSS classname fix, and an empty-state refactor that delegates to existing infrastructure (TableContainer.emptyComponent). The ManageHostsPage change is safe because the deleted block was made unreachable by the refactor — renderTable's own emptyState() factory already covers both isTrulyEmpty and filtered-empty cases, and the emptyComponent is wired through TableContainer.
Other factors
A new test in ManageHostsPage.tests.tsx exercises the filtered empty state via a low_disk_space=32 query param and asserts controls remain enabled, which guards the most likely regression from removing the old branch. Codecov reports 87.5% patch coverage. The bug hunting system found nothing.
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.
WalkthroughThis PR refactors empty-state handling and UI naming across multiple host management components. SelectReportModal's CSS prefix changes from Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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.
Actionable comments posted: 1
🧹 Nitpick comments (2)
frontend/pages/hosts/ManageHostsPage/ManageHostsPage.tests.tsx (1)
187-220: ⚡ Quick winGood coverage for the
low_disk_spacefilter pathThe test correctly validates that a query-param filter prevents the truly-empty state and keeps controls enabled.
Consider adding a negative assertion to make the "not truly empty" intent explicit, and asserting the export button is absent (count=0, not
isTrulyEmpty→ the button is intentionally hidden):💡 Suggested additions
expect( await screen.findByText("No hosts match your filters") ).toBeInTheDocument(); + // Confirm we are NOT in the truly-empty state + expect(screen.queryByText("No hosts")).not.toBeInTheDocument(); expect( screen.getByText( /Recently enrolled hosts will appear here after their first check-in/ ) ).toBeInTheDocument(); // Controls are NOT disabled expect(screen.getByPlaceholderText(/search name/i)).not.toBeDisabled(); expect( screen.getByRole("button", { name: /edit columns/i }) ).not.toBeDisabled(); + // Export button is hidden when count is 0 and not truly empty + expect( + screen.queryByRole("button", { name: /export hosts/i }) + ).not.toBeInTheDocument();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/pages/hosts/ManageHostsPage/ManageHostsPage.tests.tsx` around lines 187 - 220, Add an explicit negative assertion in the "renders filtered empty state for query param filters with enabled controls" test to show this is the filtered (not truly-empty) state: after the existing checks, assert that the export control is absent by querying for the export button/dropdown (e.g., queryAllByRole("button", { name: /export/i }) or queryByRole) and expecting zero results; this makes the intent explicit instead of relying on an `isTrulyEmpty` flag.frontend/pages/hosts/ManageHostsPage/ManageHostsPage.tsx (1)
1759-1759: 💤 Low valueLGTM — tooltip now tracks the constant
Replacing the hardcoded
"5,000"withMAX_SCRIPT_BATCH_TARGETS.toLocaleString()ensures the tooltip stays in sync if the constant ever changes.One small note:
toLocaleString()with no locale argument uses the browser's default locale (e.g.,"5.000"in German,"5,000"in English). Since all surrounding strings are hardcoded in English, you may want to pin the locale:- `Target at most ${MAX_SCRIPT_BATCH_TARGETS.toLocaleString()} hosts to run a script.` + `Target at most ${MAX_SCRIPT_BATCH_TARGETS.toLocaleString("en-US")} hosts to run a script.`This is only relevant if users ever run Fleet UI in a non-English locale.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/pages/hosts/ManageHostsPage/ManageHostsPage.tsx` at line 1759, Tooltip message now uses MAX_SCRIPT_BATCH_TARGETS.toLocaleString() which varies by user locale; change the call to use a pinned English locale so the tooltip remains English-consistent (e.g., call toLocaleString with 'en-US') when setting disableRunScriptBatchTooltipContent and any other occurrences referencing MAX_SCRIPT_BATCH_TARGETS.toLocaleString().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/pages/hosts/details/cards/Policies/HostPolicies.tsx`:
- Around line 148-153: The primaryButton currently renders the "Manage policies"
Button solely when canManagePolicies is true, which can produce a non-functional
CTA if onManagePolicies is undefined; update the conditional for primaryButton
to require both canManagePolicies and onManagePolicies before rendering the
Button (i.e., only render <Button onClick={onManagePolicies} ...> when both
canManagePolicies and onManagePolicies are truthy) so the Button's onClick
handler is always present; check the HostPolicies component's primaryButton prop
and its use of Button, canManagePolicies, and onManagePolicies to implement this
guard.
---
Nitpick comments:
In `@frontend/pages/hosts/ManageHostsPage/ManageHostsPage.tests.tsx`:
- Around line 187-220: Add an explicit negative assertion in the "renders
filtered empty state for query param filters with enabled controls" test to show
this is the filtered (not truly-empty) state: after the existing checks, assert
that the export control is absent by querying for the export button/dropdown
(e.g., queryAllByRole("button", { name: /export/i }) or queryByRole) and
expecting zero results; this makes the intent explicit instead of relying on an
`isTrulyEmpty` flag.
In `@frontend/pages/hosts/ManageHostsPage/ManageHostsPage.tsx`:
- Line 1759: Tooltip message now uses MAX_SCRIPT_BATCH_TARGETS.toLocaleString()
which varies by user locale; change the call to use a pinned English locale so
the tooltip remains English-consistent (e.g., call toLocaleString with 'en-US')
when setting disableRunScriptBatchTooltipContent and any other occurrences
referencing MAX_SCRIPT_BATCH_TARGETS.toLocaleString().
🪄 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: f9fac436-01bf-4154-bd01-cf622ae651cf
📒 Files selected for processing (6)
frontend/pages/hosts/ManageHostsPage/ManageHostsPage.tests.tsxfrontend/pages/hosts/ManageHostsPage/ManageHostsPage.tsxfrontend/pages/hosts/details/HostDetailsPage/modals/SelectReportModal/SelectReportModal.tsxfrontend/pages/hosts/details/HostDetailsPage/modals/SelectReportModal/_styles.scssfrontend/pages/hosts/details/cards/Policies/HostPolicies.tsxfrontend/pages/hosts/details/cards/Policies/_styles.scss
| primaryButton={ | ||
| canManagePolicies ? ( | ||
| <Button onClick={onManagePolicies} type="button"> | ||
| Manage policies | ||
| </Button> | ||
| ) : undefined |
There was a problem hiding this comment.
Guard the “Manage policies” CTA on callback presence.
Line 149 currently keys button rendering only on canManagePolicies. If onManagePolicies is missing, this can surface a non-functional primary action. Gate on both conditions.
Suggested fix
+ const canShowManagePoliciesCta = canManagePolicies && !!onManagePolicies;
+
return (
<>
{renderBanner()}
<TableContainer
@@
header="No policies checked"
info={`Select Refetch to load the latest data from ${target}${manageClause}`}
primaryButton={
- canManagePolicies ? (
+ canShowManagePoliciesCta ? (
<Button onClick={onManagePolicies} type="button">
Manage policies
</Button>
) : undefined
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/pages/hosts/details/cards/Policies/HostPolicies.tsx` around lines
148 - 153, The primaryButton currently renders the "Manage policies" Button
solely when canManagePolicies is true, which can produce a non-functional CTA if
onManagePolicies is undefined; update the conditional for primaryButton to
require both canManagePolicies and onManagePolicies before rendering the Button
(i.e., only render <Button onClick={onManagePolicies} ...> when both
canManagePolicies and onManagePolicies are truthy) so the Button's onClick
handler is always present; check the HostPolicies component's primaryButton prop
and its use of Button, canManagePolicies, and onManagePolicies to implement this
guard.
There was a problem hiding this comment.
Pull request overview
This PR follows up on the Fleet UI empty-state normalization work by tightening up empty-state behavior across host management and host details UI, and aligning a few implementation details with the updated patterns introduced in prior PRs.
Changes:
- Manage hosts: remove a special-case early-return empty state so query-param filtered “empty” results still render the table chrome (controls remain enabled), and add test coverage for this scenario.
- Host details: move the “No policies checked” empty state into the
TableContainer’semptyComponentso the empty-state behavior is consistent with other tables. - Fix minor UI consistency issues (e.g., SelectReportModal base CSS class naming; run-script batch tooltip now reflects
MAX_SCRIPT_BATCH_TARGETS).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/pages/hosts/ManageHostsPage/ManageHostsPage.tsx | Removes an early-return empty-state branch so filtered empty results render through TableContainer, and updates script batch tooltip copy to use the shared constant. |
| frontend/pages/hosts/ManageHostsPage/ManageHostsPage.tests.tsx | Adds a test ensuring query-param filters (e.g., low_disk_space) show the filtered empty state while keeping controls enabled. |
| frontend/pages/hosts/details/HostDetailsPage/modals/SelectReportModal/SelectReportModal.tsx | Renames the modal base class to match the component intent and associated styles. |
| frontend/pages/hosts/details/HostDetailsPage/modals/SelectReportModal/_styles.scss | Minor formatting; styles target the corrected base class. |
| frontend/pages/hosts/details/cards/Policies/HostPolicies.tsx | Consolidates the “No policies checked” empty state into TableContainer.emptyComponent to normalize behavior. |
| frontend/pages/hosts/details/cards/Policies/_styles.scss | Removes hiding of the table header so the standard TableContainer header/count can render. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Issue
Follow ups to #35483
Description
Addresses:
Testing
Summary by CodeRabbit