Fleet UI: Update empty states for host-related modals#44940
Fleet UI: Update empty states for host-related modals#44940RachelElysia merged 5 commits intomainfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #44940 +/- ##
==========================================
- Coverage 66.78% 66.76% -0.02%
==========================================
Files 2688 2693 +5
Lines 216986 216784 -202
Branches 9963 9983 +20
==========================================
- Hits 144904 144727 -177
+ Misses 58932 58922 -10
+ Partials 13150 13135 -15
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:
|
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| <> | ||
| <CustomLink | ||
| url={getPathWithQueryParams(PATHS.NEW_REPORT, { | ||
| fleet_id: currentTeamId, | ||
| })} | ||
| text="Create a report" |
There was a problem hiding this comment.
🔴 The empty-state "Create a report" link at SelectReportModal.tsx:175-181 only passes fleet_id and drops host_id, while the description-bar create a report button (line 153) calls onRunCustomReport (lines 75-83) which uses router.push with both host_id and fleet_id plus resets QueryContext targets via setSelectedQueryTargetsByType(DEFAULT_TARGETS_BY_TYPE). Because CustomLink renders a plain <a href> (full-page navigation), users opening the modal on a host with no saved reports lose the host context — the resulting /reports/new page no longer knows which host they came from. To fix, replace the CustomLink with a Button variant="link" that calls onRunCustomReport to match the description-bar path.
Extended reasoning...
What the bug is
SelectReportModal exposes two paths to create a new report from the current host context, and they behave differently:
-
Description-bar button (
SelectReportModal.tsx:152-156):<Button variant="link" onClick={onRunCustomReport}>create a report</Button>.onRunCustomReport(lines 75-83) callssetSelectedQueryTargetsByType(DEFAULT_TARGETS_BY_TYPE)to reset QueryContext targets, thenrouter.push(getPathWithQueryParams(PATHS.NEW_REPORT, { host_id: hostId, fleet_id: currentTeamId })). -
Empty-state link (
SelectReportModal.tsx:175-181):<CustomLink url={getPathWithQueryParams(PATHS.NEW_REPORT, { fleet_id: currentTeamId })} text="Create a report" />. This omitshost_id, skips the QueryContext target reset, and — becauseCustomLinkrenders a plain anchor (frontend/components/CustomLink/CustomLink.tsx:94:<a href={url} ...>) — triggers a full-page navigation rather than a client-siderouter.push.
Why host_id matters
EditQueryPage (the destination at /reports/new) reads it at frontend/pages/queries/edit/EditQueryPage.tsx:56-58:
const hostId = location.query.host_id
? parseInt(location.query.host_id as string, 10)
: undefined;and uses hostId to (a) set the back-button label/path to "Back to host details" → PATHS.HOST_DETAILS(hostId) (otherwise it falls back to PATHS.MANAGE_REPORTS), and (b) propagate host_id into PATHS.REPORT_DETAILS after creation so the new report opens scoped to the host (lines ~270, 379). Drop host_id and you silently lose both behaviors — the user lands in a generic-looking new-report page with no breadcrumb back to the host they came from, and after creating, the post-redirect URL has hostId=undefined.
Step-by-step proof
Concrete walkthrough on a host that has zero saved reports:
- Admin opens host details for host
42and clicks "Query host" →HostDetailsPageopensSelectReportModalwithhostId=42,currentTeamId=3. - The modal's
useQueryreturns an empty list, soreportsCount === 0and we render the empty-stateEmptyStateat line 169. canCreateReportis true (admin), so theCustomLinkat lines 176-181 is rendered withurl = getPathWithQueryParams(PATHS.NEW_REPORT, { fleet_id: 3 })→/reports/new?fleet_id=3. Note: nohost_id.- Admin clicks the link. Because
CustomLinkis<a href="/reports/new?fleet_id=3">, the browser does a full page navigation to that URL. EditQueryPagemounts, runs line 56 →location.query.host_idisundefined→hostId = undefined.- As a result: back button reads "Back to reports" →
PATHS.MANAGE_REPORTS, not "Back to host details" →PATHS.HOST_DETAILS(42). Post-create redirect toPATHS.REPORT_DETAILSis built withhostId=undefined. QueryContext still holds whatever targets were last selected (no reset).
Compare to clicking the "create a report" button in the description bar of the same modal: onRunCustomReport resets targets and router.push-es to /reports/new?host_id=42&fleet_id=3 — back button correctly labels "Back to host details", and the host context survives.
Why existing code doesn't prevent it
- The new test in
SelectReportModal.tests.tsx("renders empty state with Create a report link when user can create") only asserts the link text is in the DOM viascreen.getByText("Create a report"). It never checks thehrefvalue or navigation behavior, so this divergence is not caught. - There's no shared helper that builds the
NEW_REPORTURL — each call site builds its own params, which is exactly how the two diverged.
Addressing the refutation
The refuter argued this is duplicate/nit because (a) pre-PR there was no link at all, (b) the semantic difference might be intentional ("create AND run on this host" vs "go set one up"), and (c) host_id only affects back-button label and post-create URL. Those are reasonable points, but: the link is new in this PR, both UI elements live in the same modal, both share the same label semantics in user-visible terms ("create a report" / "Create a report"), and the description-bar button is even gated by the same canCreateReport predicate as the empty-state link — they are clearly intended to be parallel affordances, not deliberately divergent ones. Losing the back-link to host details and triggering a full-page reload from inside a modal is a real UX regression on the empty-state path that admins will hit precisely when the modal is most useful (no reports yet, time to make one).
How to fix
Replace the CustomLink with a Button variant="link" that calls onRunCustomReport, mirroring the description-bar:
<Button variant="link" onClick={onRunCustomReport}>Create a report</Button>{" "}to run.That single change preserves host_id, resets QueryContext targets, and uses router.push (no full-page reload), aligning the two paths.
| @@ -39,17 +40,17 @@ export interface ISelectQueryModalProps { | |||
|
|
|||
| const baseClass = "select-query-modal"; | |||
There was a problem hiding this comment.
🟡 Nit: the component was renamed from SelectQueryModal to SelectReportModal (file, directory, interface, and exports all updated), but baseClass at line 41 and the matching root selector in _styles.scss still read select-query-modal. Functionally this works (TSX and SCSS still agree), but it leaves the rename half-finished — future searches for select-report-modal will find nothing. Consider updating both to select-report-modal to match the component name.
Extended reasoning...
What this is
The component, file, directory, interface (ISelectQueryModalProps → ISelectReportModalProps), and default export were all renamed from SelectQueryModal to SelectReportModal in this PR. However, two related strings were not updated:
SelectReportModal.tsx:41—const baseClass = "select-query-modal"_styles.scss:2— root selector.select-query-modal { ... }
Why it's not a functional bug
The baseClass string is what's interpolated into the rendered JSX className (e.g. ${baseClass}__description, ${baseClass}__query-selection, etc.) and into the SCSS BEM blocks. Since both the TSX and the SCSS still use the same string select-query-modal, every selector still matches, and the modal styles correctly. No user-visible behavior changes.
Why it's still worth flagging
The Fleet frontend convention (fleet-frontend.md: "Define const baseClass = component-name at the top of the component") is that baseClass should match the component name in BEM-style kebab-case. After this PR, SelectReportModal carries a baseClass of select-query-modal, which violates that rule. A quick grep confirms: select-query-modal exists in the new SelectReportModal/ directory (both .tsx and .scss), and select-report-modal returns zero matches anywhere. So a future engineer trying to locate the styles for SelectReportModal by searching for select-report-modal will come up empty and may waste time before realizing the older name is still in use.
Addressing the refutation
A previous review noted this is style/cosmetic-only and that the author may have intentionally kept the SCSS class stable to avoid touching unrelated styles in an empty-state PR. That's fair — the SCSS is already being modified in this PR (lines 2–7 change min-height/height to max-height), so updating the root selector at the same time is essentially free, and the baseClass change in TSX is a one-line edit. Given that the rename was thorough everywhere else (file path, directory, interface, exports, internal handler names like onQueryHostCustom → onRunCustomReport), it reads as an oversight rather than an intentional carve-out. Marking as nit so it's not blocking, but flagging in case the author wants to finish the rename in this PR rather than leaving it for a future grep-frustrated reader.
Step-by-step proof
- Open
SelectReportModal.tsxafter the PR. Line 41 readsconst baseClass = "select-query-modal";. - Open
_styles.scssafter the PR. Line 2 reads.select-query-modal {. - Run
grep -rn select-report-modal frontend/→ zero matches. - Run
grep -rn select-query-modal frontend/→ matches only the two lines above (both inside the newSelectReportModal/directory). - Conclusion: the component named
SelectReportModalships with a CSS root class namedselect-query-modal, and no code anywhere referencesselect-report-modal.
Suggested fix
- const baseClass = "select-query-modal";
+ const baseClass = "select-report-modal";- .select-query-modal {
+ .select-report-modal {No other references need updating, since every internal class name is built from ${baseClass} interpolation.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?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.
WalkthroughThis PR introduces 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 3
🧹 Nitpick comments (1)
frontend/pages/hosts/ManageHostsPage/components/RunScriptBatchModal/RunScriptBatchModal.tests.tsx (1)
152-163: 💤 Low valueGood coverage of the empty-state path; consider querying by role for the link.
screen.getByText("Add a script")works, butscreen.getByRole("link", { name: "Add a script" })is more semantically precise and verifies thatCustomLinkactually renders an accessible<a>element — catching any future regression where the link is replaced with a non-interactive element.♻️ Proposed refinement
- expect(screen.getByText("Add a script")).toBeInTheDocument(); + expect(screen.getByRole("link", { name: "Add a script" })).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/components/RunScriptBatchModal/RunScriptBatchModal.tests.tsx` around lines 152 - 163, Update the test "renders empty state with Add a script link when no scripts exist" in RunScriptBatchModal.tests.tsx to assert the Add a script element by role rather than plain text: replace the screen.getByText("Add a script") assertion with screen.getByRole("link", { name: "Add a script" }) so the test verifies that RunScriptBatchModal (and its CustomLink) renders an actual accessible anchor element instead of a non-interactive element.
🤖 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/HostDetailsPage/modals/SelectReportModal/_styles.scss`:
- Around line 11-12: Add a blank line before the height declaration to satisfy
stylelint's declaration-empty-line-before rule: locate the block that contains
"@include vertical-modal-layout;" and the "height: 100%;" declaration in
_styles.scss and insert a single empty line between them so the mixin call and
the "height: 100%;" rule are separated.
In
`@frontend/pages/hosts/details/HostDetailsPage/modals/SelectReportModal/SelectReportModal.tsx`:
- Line 40: The component SelectReportModal still sets the CSS prefix variable
baseClass = "select-query-modal" so it continues to render old selectors; change
baseClass to "select-report-modal" and update any hardcoded uses of
"select-query-modal" in the component (references inside SelectReportModal
JSX/JS where classes are built) to use the new value so the migrated
.select-report-modal styles apply (also update the other occurrences noted in
the file to the new selector).
In `@frontend/pages/hosts/ManageHostsPage/ManageHostsPage.tsx`:
- Around line 1786-1789: The tooltip text is hardcoded to "5,000" while the
condition uses the MAX_SCRIPT_BATCH_TARGETS constant, causing drift if the
constant changes; update the assignment to disableRunScriptBatchTooltipContent
to construct the message using MAX_SCRIPT_BATCH_TARGETS (e.g., interpolate or
format the constant into the string) instead of the literal "5,000" so the
tooltip always reflects the current MAX_SCRIPT_BATCH_TARGETS value referenced in
the condition.
---
Nitpick comments:
In
`@frontend/pages/hosts/ManageHostsPage/components/RunScriptBatchModal/RunScriptBatchModal.tests.tsx`:
- Around line 152-163: Update the test "renders empty state with Add a script
link when no scripts exist" in RunScriptBatchModal.tests.tsx to assert the Add a
script element by role rather than plain text: replace the screen.getByText("Add
a script") assertion with screen.getByRole("link", { name: "Add a script" }) so
the test verifies that RunScriptBatchModal (and its CustomLink) renders an
actual accessible anchor element instead of a non-interactive element.
🪄 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: 3d1ad363-58bd-4833-81b3-eec9c19cc809
📒 Files selected for processing (9)
frontend/pages/hosts/ManageHostsPage/ManageHostsPage.tsxfrontend/pages/hosts/ManageHostsPage/components/RunScriptBatchModal/RunScriptBatchModal.tests.tsxfrontend/pages/hosts/ManageHostsPage/components/RunScriptBatchModal/RunScriptBatchModal.tsxfrontend/pages/hosts/details/HostDetailsPage/HostDetailsPage.tsxfrontend/pages/hosts/details/HostDetailsPage/modals/SelectQueryModal/index.tsfrontend/pages/hosts/details/HostDetailsPage/modals/SelectReportModal/SelectReportModal.tests.tsxfrontend/pages/hosts/details/HostDetailsPage/modals/SelectReportModal/SelectReportModal.tsxfrontend/pages/hosts/details/HostDetailsPage/modals/SelectReportModal/_styles.scssfrontend/pages/hosts/details/HostDetailsPage/modals/SelectReportModal/index.ts
💤 Files with no reviewable changes (1)
- frontend/pages/hosts/details/HostDetailsPage/modals/SelectQueryModal/index.ts
| @include vertical-modal-layout; | ||
| height: 100%; |
There was a problem hiding this comment.
Fix stylelint spacing before height declaration.
Stylelint flags declaration-empty-line-before here; add a blank line before height: 100%; to keep CI green.
Proposed fix
.modal__content {
`@include` vertical-modal-layout;
+
height: 100%;
}🧰 Tools
🪛 Stylelint (17.10.0)
[error] 12-12: Expected empty line before declaration (declaration-empty-line-before)
(declaration-empty-line-before)
🤖 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/HostDetailsPage/modals/SelectReportModal/_styles.scss`
around lines 11 - 12, Add a blank line before the height declaration to satisfy
stylelint's declaration-empty-line-before rule: locate the block that contains
"@include vertical-modal-layout;" and the "height: 100%;" declaration in
_styles.scss and insert a single empty line between them so the mixin call and
the "height: 100%;" rule are separated.
| @@ -39,17 +39,17 @@ export interface ISelectQueryModalProps { | |||
|
|
|||
| const baseClass = "select-query-modal"; | |||
There was a problem hiding this comment.
baseClass still points to old selector, so new modal styles won’t apply.
_styles.scss was migrated to .select-report-modal, but this component still renders select-query-modal classes. That breaks the intended empty-state layout/styling updates.
Proposed fix
-const baseClass = "select-query-modal";
+const baseClass = "select-report-modal";Also applies to: 218-218, 252-252
🤖 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/HostDetailsPage/modals/SelectReportModal/SelectReportModal.tsx`
at line 40, The component SelectReportModal still sets the CSS prefix variable
baseClass = "select-query-modal" so it continues to render old selectors; change
baseClass to "select-report-modal" and update any hardcoded uses of
"select-query-modal" in the component (references inside SelectReportModal
JSX/JS where classes are built) to use the new value so the migrated
.select-report-modal styles apply (also update the other occurrences noted in
the file to the new selector).
| totalFilteredHostsCount > MAX_SCRIPT_BATCH_TARGETS | ||
| ) { | ||
| disableRunScriptBatchTooltipContent = | ||
| "Target at most 5,000 hosts to run a script"; | ||
| "Target at most 5,000 hosts to run a script."; |
There was a problem hiding this comment.
Hardcoded "5,000" will drift from MAX_SCRIPT_BATCH_TARGETS constant.
The condition on line 1786 uses MAX_SCRIPT_BATCH_TARGETS, but the tooltip on line 1789 hardcodes "5,000". If the constant changes the tooltip becomes silently stale.
🛠️ Proposed fix
- "Target at most 5,000 hosts to run a script.";
+ `Target at most ${MAX_SCRIPT_BATCH_TARGETS.toLocaleString()} hosts to run a script.`;📝 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.
| totalFilteredHostsCount > MAX_SCRIPT_BATCH_TARGETS | |
| ) { | |
| disableRunScriptBatchTooltipContent = | |
| "Target at most 5,000 hosts to run a script"; | |
| "Target at most 5,000 hosts to run a script."; | |
| totalFilteredHostsCount > MAX_SCRIPT_BATCH_TARGETS | |
| ) { | |
| disableRunScriptBatchTooltipContent = | |
| `Target at most ${MAX_SCRIPT_BATCH_TARGETS.toLocaleString()} hosts to run a script.`; |
🤖 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` around lines 1786 -
1789, The tooltip text is hardcoded to "5,000" while the condition uses the
MAX_SCRIPT_BATCH_TARGETS constant, causing drift if the constant changes; update
the assignment to disableRunScriptBatchTooltipContent to construct the message
using MAX_SCRIPT_BATCH_TARGETS (e.g., interpolate or format the constant into
the string) instead of the literal "5,000" so the tooltip always reflects the
current MAX_SCRIPT_BATCH_TARGETS value referenced in the condition.
There was a problem hiding this comment.
This is an unrelated released bug, it's hardcoded in the constant, just going to point to the constant since it's a quick fix.
There was a problem hiding this comment.
Pull request overview
Updates host-related modals to use normalized/consistent empty states as part of the ongoing Fleet UI empty-state normalization effort (#35483).
Changes:
- Adds/updates empty-state copy and CTAs for “Run script” batch modal and host “Select report” modal.
- Renames “SelectQueryModal” to “SelectReportModal” throughout Host details flow and introduces a new test suite for the modal.
- Minor tooltip copy punctuation tweaks.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/pages/hosts/ManageHostsPage/ManageHostsPage.tsx | Tooltip copy punctuation updates for run-script batch action. |
| frontend/pages/hosts/ManageHostsPage/components/RunScriptBatchModal/RunScriptBatchModal.tsx | Updates “no scripts” empty state text/CTA link. |
| frontend/pages/hosts/ManageHostsPage/components/RunScriptBatchModal/RunScriptBatchModal.tests.tsx | Adds coverage for the updated “no scripts” empty state. |
| frontend/pages/hosts/details/HostDetailsPage/modals/SelectReportModal/SelectReportModal.tsx | Renames modal semantics to “report” and updates empty-state behavior/CTA. |
| frontend/pages/hosts/details/HostDetailsPage/modals/SelectReportModal/SelectReportModal.tests.tsx | Adds tests for SelectReportModal empty states and list rendering. |
| frontend/pages/hosts/details/HostDetailsPage/modals/SelectReportModal/index.ts | New barrel export for SelectReportModal. |
| frontend/pages/hosts/details/HostDetailsPage/modals/SelectReportModal/_styles.scss | Renames modal CSS selector and adjusts height/scroll styling. |
| frontend/pages/hosts/details/HostDetailsPage/modals/SelectQueryModal/index.ts | Removes old SelectQueryModal barrel export. |
| frontend/pages/hosts/details/HostDetailsPage/HostDetailsPage.tsx | Wires Host details “query” action to SelectReportModal. |
Comments suppressed due to low confidence (3)
frontend/pages/hosts/details/HostDetailsPage/modals/SelectReportModal/SelectReportModal.tsx:42
baseClassis still set to "select-query-modal", but the corresponding SCSS defines styles under.select-report-modal. As a result, the modal won't pick up its intended styling. UpdatebaseClass(and the ModalclassName) to match the SCSS selector, or rename the SCSS selector to match the component.
frontend/pages/hosts/details/HostDetailsPage/modals/SelectReportModal/SelectReportModal.tsx:55- The
useQuerykey/params in this block ultimately derive theteamIdfromhostTeamId. SincehostTeamIdcan benullfor unassigned hosts (IHost.team_id), ensure the query uses the API’s “no team” value (e.g.API_NO_TEAM_ID) rather than falling back toundefined(which typically means “All fleets”). Using||with numeric IDs can also mis-handle0if it’s ever passed.
frontend/pages/hosts/details/HostDetailsPage/HostDetailsPage.tsx:1587 host.team_idisnumber | null(unassigned hosts usenull), butSelectReportModaluses this value to scope which reports are selectable. Passingnullhere will typically be treated as “All fleets” by the reports API. Consider coercing to the API’s “Unassigned” fleet id (e.g.host.team_id ?? API_NO_TEAM_ID) like other parts of this page already do when passinghostTeamId.
isOnlyObserver={isOnlyObserver}
hostId={hostIdFromURL}
hostTeamId={host?.team_id}
router={router}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Small followup PR incoming |
Issue
Closes #35483
Description
Screenshots of updates
Testing
Summary by CodeRabbit
New Features
Bug Fixes
Tests