Fleet UI: Update empty states in Controls - #44965
Conversation
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #44965 +/- ##
==========================================
- Coverage 66.78% 66.76% -0.03%
==========================================
Files 2718 2727 +9
Lines 218769 219193 +424
Branches 10745 10912 +167
==========================================
+ Hits 146109 146345 +236
- Misses 59496 59686 +190
+ Partials 13164 13162 -2
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}
76752e3 to
967c531
Compare
|
@claude review |
|
@claude review |
There was a problem hiding this comment.
LGTM — all prior feedback addressed; the remaining inline comment is a pre-existing minor UX nit.
Extended reasoning...
Overview
This PR updates empty states across the Controls section (Scripts library, Batch progress, Bootstrap package, Variables) and renames the Secrets domain to Variables (interface, service, state, copy). It's primarily UI/UX copy and structural changes plus a mechanical rename, with a small route addition for a learn-more link.
Security risks
None identified. No auth, crypto, permissions, or input-handling code is touched. The Variables rename preserves the same backend endpoint (/fleet/custom_variables), API surface, and access semantics. Role gating (isTechnician, canEdit) is preserved and now consistent between empty/populated states. The new GitOps tooltip wrapper around the Upload button matches the existing pattern.
Level of scrutiny
Moderate — frontend-only, narrowly scoped to ManageControls pages. No CODEOWNER-sensitive code, no backend changes, no schema migrations. The Secrets→Variables rename is mechanical but touches multiple files; it's been done consistently and tests were updated alongside.
Other factors
- All prior review feedback has been addressed: the
getWhennamed-import fix landed inScriptBatchDetailsPage.tsx, thecanUploadScriptspredicate was simplified to!isTechnicianand now matches the populated-state heading, the Upload CTA is wrapped inGitOpsModeTooltipWrapper, and the two stale 'secret' error strings inAddCustomVariableModalwere renamed to 'variable'. - New tests were added for both ScriptLibrary empty-state role variants and Variables read-only/edit empty states, including a GitOps-mode test for Variables.
- The remaining inline comment flags a pre-existing bug (EmptyState rendering alongside DataError when the scripts API errors) that this PR magnifies visually but did not introduce; the author can address it as a follow-up.
1c3b751 to
f6b86c6
Compare
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.
|
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:
WalkthroughThis PR renames and repurposes the custom secrets flow to custom variables: new IVariable/IVariablePayload interfaces, a variables service with get/add/delete, endpoint key rename SECRETS→VARIABLES, refactored Variables page and add/delete modals, removed legacy secrets service, updated styles/tests and mock responses. Separately, script helpers and components normalize empty states to JSX (learn-more links, uploader text), convert getWhen to a named export, add a batch-scripts redirect route, and update setup-card empty-state copy about enabling MDM and automatic enrollment. Possibly related PRs
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/pages/ManageControlsPage/Variables/Variables.tests.tsx (1)
117-123:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse dynamic
countin the mock GET response.
countis tied tomockSecrets.length(static), but add/delete mutatesecretsResponse.secrets. This can mask list/pagination regressions in tests after mutations.💡 Suggested test fix
const secretsHandler = http.get(baseUrl("/custom_variables"), () => { return HttpResponse.json({ custom_variables: secretsResponse.secrets, - count: mockSecrets.length, + count: secretsResponse.secrets.length, has_prev_results: false, has_next_results: false, }); });🤖 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/ManageControlsPage/Variables/Variables.tests.tsx` around lines 117 - 123, The mock GET response currently uses a static count (mockSecrets.length) which doesn't reflect runtime mutations to secretsResponse.secrets; update the mock response returned by the HttpResponse.json call to compute count from the live array (e.g., use secretsResponse.secrets.length) so adds/deletes in tests change the returned count and pagination behavior; locate the mock handler that calls HttpResponse.json and replace the static reference to mockSecrets with a dynamic length derived from secretsResponse.secrets.
🤖 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.
Outside diff comments:
In `@frontend/pages/ManageControlsPage/Variables/Variables.tests.tsx`:
- Around line 117-123: The mock GET response currently uses a static count
(mockSecrets.length) which doesn't reflect runtime mutations to
secretsResponse.secrets; update the mock response returned by the
HttpResponse.json call to compute count from the live array (e.g., use
secretsResponse.secrets.length) so adds/deletes in tests change the returned
count and pagination behavior; locate the mock handler that calls
HttpResponse.json and replace the static reference to mockSecrets with a dynamic
length derived from secretsResponse.secrets.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: eab7aed5-bf30-4bfc-9d86-d327c8df04fd
📒 Files selected for processing (19)
frontend/interfaces/variables.tsfrontend/pages/ManageControlsPage/Scripts/ScriptBatchDetailsPage/ScriptBatchDetailsPage.tsxfrontend/pages/ManageControlsPage/Scripts/cards/ScriptBatchProgress/ScriptBatchProgress.tests.tsxfrontend/pages/ManageControlsPage/Scripts/cards/ScriptBatchProgress/ScriptBatchProgress.tsxfrontend/pages/ManageControlsPage/Scripts/cards/ScriptLibrary/ScriptLibrary.tests.tsxfrontend/pages/ManageControlsPage/Scripts/cards/ScriptLibrary/ScriptLibrary.tsxfrontend/pages/ManageControlsPage/Scripts/components/ScriptUploader/ScriptUploader.tsxfrontend/pages/ManageControlsPage/Scripts/helpers.tsxfrontend/pages/ManageControlsPage/SetupExperience/cards/BootstrapPackage/BootstrapPackage.tests.tsxfrontend/pages/ManageControlsPage/SetupExperience/cards/BootstrapPackage/BootstrapPackage.tsxfrontend/pages/ManageControlsPage/Variables/Variables.tests.tsxfrontend/pages/ManageControlsPage/Variables/Variables.tsxfrontend/pages/ManageControlsPage/Variables/_styles.scssfrontend/pages/ManageControlsPage/Variables/components/AddCustomVariableModal/AddCustomVariableModal.tsxfrontend/pages/ManageControlsPage/Variables/components/DeleteCustomVariableModal/DeleteCustomVariableModal.tsxfrontend/services/entities/secrets.tsxfrontend/services/entities/variables.tsxfrontend/utilities/endpoints.tswebsite/config/routes.js
💤 Files with no reviewable changes (1)
- frontend/services/entities/secrets.tsx
There was a problem hiding this comment.
Pull request overview
Updates several “Controls” UI surfaces to use the normalized empty-state patterns (copy, CTAs, and learn-more links), and refactors the “custom variables” frontend API naming from “secrets” to “variables”.
Changes:
- Normalized empty states and CTAs across Controls (Custom variables, Scripts library, Batch script progress, Bootstrap package).
- Refactored custom variables frontend plumbing: renamed endpoints key, replaced
secretsentity service withvariables, and updated related UI/components/tests. - Added a website redirect for the new “Learn more about batch scripts” link target.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| website/config/routes.js | Adds /learn-more-about/batch-scripts redirect used by the new batch-scripts learn-more links. |
| frontend/utilities/endpoints.ts | Renames the custom variables endpoint key from SECRETS to VARIABLES. |
| frontend/services/entities/variables.tsx | New entity service for listing/creating/deleting custom variables via /custom_variables. |
| frontend/services/entities/secrets.tsx | Removes old custom-variables service named secrets. |
| frontend/pages/ManageControlsPage/Variables/Variables.tsx | Updates custom variables page copy/empty state behavior and swaps to variablesAPI + IVariable. |
| frontend/pages/ManageControlsPage/Variables/Variables.tests.tsx | Updates tests for the custom variables page, including new read-only empty state expectations. |
| frontend/pages/ManageControlsPage/Variables/components/DeleteCustomVariableModal/DeleteCustomVariableModal.tsx | Swaps modal to variable naming and variablesAPI.deleteVariable. |
| frontend/pages/ManageControlsPage/Variables/components/AddCustomVariableModal/AddCustomVariableModal.tsx | Swaps modal to variable naming and variablesAPI.addVariable, updates flash messages. |
| frontend/pages/ManageControlsPage/Variables/_styles.scss | Renames CSS selectors to match updated variables page class names. |
| frontend/pages/ManageControlsPage/SetupExperience/cards/BootstrapPackage/BootstrapPackage.tsx | Updates Bootstrap package empty state copy to the normalized format. |
| frontend/pages/ManageControlsPage/SetupExperience/cards/BootstrapPackage/BootstrapPackage.tests.tsx | Updates assertions to match new Bootstrap package empty state copy. |
| frontend/pages/ManageControlsPage/Scripts/ScriptBatchDetailsPage/ScriptBatchDetailsPage.tsx | Updates getWhen import after helpers refactor (named export). |
| frontend/pages/ManageControlsPage/Scripts/helpers.tsx | Adds shared script-uploader empty-state text constants and converts getWhen to named export. |
| frontend/pages/ManageControlsPage/Scripts/components/ScriptUploader/ScriptUploader.tsx | Uses the shared SCRIPT_UPLOADER_TEXT constant for consistent copy. |
| frontend/pages/ManageControlsPage/Scripts/cards/ScriptLibrary/ScriptLibrary.tsx | Replaces empty-state uploader UI with normalized EmptyState + Upload CTA + GitOps wrapper. |
| frontend/pages/ManageControlsPage/Scripts/cards/ScriptLibrary/ScriptLibrary.tests.tsx | Adds automated coverage for Script Library empty state role-based CTA visibility. |
| frontend/pages/ManageControlsPage/Scripts/cards/ScriptBatchProgress/ScriptBatchProgress.tsx | Updates batch progress empty states (copy + learn-more link) and header text. |
| frontend/pages/ManageControlsPage/Scripts/cards/ScriptBatchProgress/ScriptBatchProgress.tests.tsx | Updates tests to match the new empty state rendering (text assertions). |
| frontend/interfaces/variables.ts | Renames ISecret/ISecretPayload to IVariable/IVariablePayload. |
Comments suppressed due to low confidence (1)
frontend/pages/ManageControlsPage/Variables/Variables.tests.tsx:122
- In this list /custom_variables handler, the mocked response doesn’t match the real API shape:
- Pagination should be under
meta: { has_next_results, has_previous_results }(not top-levelhas_prev_results/has_next_results). countshould reflect the current number of returned variables (e.g.secretsResponse.secrets.length), otherwise add/delete flows can behave differently than production when the list becomes empty.
const secretsHandler = http.get(baseUrl("/custom_variables"), () => {
return HttpResponse.json({
custom_variables: secretsResponse.secrets,
count: mockSecrets.length,
has_prev_results: false,
has_next_results: false,
});
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/ManageControlsPage/SetupExperience/cards/SetupAssistant/SetupAssistant.tests.tsx`:
- Around line 56-58: Prettier formatting requires the regex argument to
screen.getByText to be on the same line; in SetupAssistant.tests.tsx locate the
assertion using screen.getByText(/Turn on MDM and automatic enrollment to
customize/) and put the regex directly inside the parentheses (i.e.,
screen.getByText(/Turn on MDM and automatic enrollment to customize/)) so the
pattern isn't on a separate line.
🪄 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: 5c2000e0-52e0-4fd3-bfdd-d1e95bed8892
📥 Commits
Reviewing files that changed from the base of the PR and between b70445c and c9d7794a070718f55f677d28108d971d0bc4099b.
📒 Files selected for processing (1)
frontend/pages/ManageControlsPage/SetupExperience/cards/SetupAssistant/SetupAssistant.tests.tsx
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
frontend/pages/ManageControlsPage/Variables/Variables.tests.tsx (1)
137-139: ⚡ Quick winGenerate new mock IDs from current state, not a fixed seed.
Line 138 always derives from
mockVariables.length, which can duplicate IDs if more than one variable is added in a test.Suggested fix
- const newVariable = { - id: mockVariables.length + 1, + const newVariable = { + id: (variablesResponse.variables.at(-1)?.id ?? 0) + 1,🤖 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/ManageControlsPage/Variables/Variables.tests.tsx` around lines 137 - 139, The test creates newVariable with id computed as mockVariables.length + 1 which can produce duplicate IDs when multiple variables are added; change the ID generation in the test helper so it derives from the current IDs (e.g. compute max ID from mockVariables and add 1, or use a unique generator like Date.now()/a UUID) instead of using mockVariables.length; update the creation site referenced as newVariable and any test helpers that append to mockVariables so each new entry uses the computed unique id.
🤖 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/ManageControlsPage/Variables/Variables.tests.tsx`:
- Around line 120-122: The test mock returns metadata count that can drift from
the actual payload; change the mock to derive count from the payload instead of
a fixed value. Replace the hardcoded count (currently using
mockVariables.length) with a dynamic value computed from the returned array
(e.g., count: variablesResponse.variables.length or compute from
custom_variables) so count always reflects the actual custom_variables in
variablesResponse.
In `@frontend/services/mock_service/mocks/responses.ts`:
- Around line 10645-10650: The new variable ID is being pre-incremented using
(nextVariableId += 1) which causes an off-by-one gap; change the assignment so
the current nextVariableId is used and then incremented (e.g., replace
(nextVariableId += 1) with nextVariableId++) when creating the object in the
mockVariables array (the entry containing name: secret.name, id: ...,
created_at: ...), ensuring nextVariableId increments after assignment so IDs
start at the expected value.
---
Nitpick comments:
In `@frontend/pages/ManageControlsPage/Variables/Variables.tests.tsx`:
- Around line 137-139: The test creates newVariable with id computed as
mockVariables.length + 1 which can produce duplicate IDs when multiple variables
are added; change the ID generation in the test helper so it derives from the
current IDs (e.g. compute max ID from mockVariables and add 1, or use a unique
generator like Date.now()/a UUID) instead of using mockVariables.length; update
the creation site referenced as newVariable and any test helpers that append to
mockVariables so each new entry uses the computed unique id.
🪄 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: f77765bd-368f-4639-9064-d2662fb49145
📥 Commits
Reviewing files that changed from the base of the PR and between c9d7794a070718f55f677d28108d971d0bc4099b and e30bd493ccc74feac06adfc9496ebc1a4c7920eb.
📒 Files selected for processing (6)
frontend/pages/ManageControlsPage/Scripts/cards/ScriptBatchProgress/ScriptBatchProgress.tsxfrontend/pages/ManageControlsPage/SetupExperience/cards/SetupAssistant/SetupAssistant.tests.tsxfrontend/pages/ManageControlsPage/Variables/Variables.tests.tsxfrontend/pages/ManageControlsPage/Variables/components/AddCustomVariableModal/AddCustomVariableModal.tsxfrontend/services/mock_service/mocks/config.tsfrontend/services/mock_service/mocks/responses.ts
✅ Files skipped from review due to trivial changes (1)
- frontend/pages/ManageControlsPage/SetupExperience/cards/SetupAssistant/SetupAssistant.tests.tsx
| mockVariables = [ | ||
| ...mockVariables, | ||
| { | ||
| name: secret.name, | ||
| id: (nextSecretId += 1), | ||
| id: (nextVariableId += 1), | ||
| created_at: new Date().toISOString(), |
There was a problem hiding this comment.
Fix off-by-one ID assignment for newly added variables.
Line 10649 increments before assignment, so the first created variable skips an ID (starts at 41 instead of 40).
Suggested fix
- id: (nextVariableId += 1),
+ id: nextVariableId++,🤖 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/services/mock_service/mocks/responses.ts` around lines 10645 -
10650, The new variable ID is being pre-incremented using (nextVariableId += 1)
which causes an off-by-one gap; change the assignment so the current
nextVariableId is used and then incremented (e.g., replace (nextVariableId += 1)
with nextVariableId++) when creating the object in the mockVariables array (the
entry containing name: secret.name, id: ..., created_at: ...), ensuring
nextVariableId increments after assignment so IDs start at the expected value.
9b1a12f to
8e8eff1
Compare
| @@ -1228,6 +1201,7 @@ module.exports.routes = { | |||
| 'GET /learn-more-about/exe-install-scripts': '/guides/exe-install-scripts', | |||
| 'GET /learn-more-about/install-scripts': '/guides/deploy-software-packages#install-script', | |||
| 'GET /learn-more-about/uninstall-scripts': '/guides/deploy-software-packages#uninstall-script', | |||
| 'GET /learn-more-about/batch-scripts': '/guides/scripts#batch-execute-scripts', | |||
| nextVariableId += 1; | ||
| mockVariables = [ | ||
| ...mockVariables, | ||
| { | ||
| name: secret.name, | ||
| id: (nextSecretId += 1), | ||
| name: variable.name, | ||
| id: nextVariableId, | ||
| created_at: new Date().toISOString(), |
There was a problem hiding this comment.
🟡 🟣 The CodeRabbit suggestion at line 10649 was marked "fixed" in commit 9b1a12f, but the rewrite (nextVariableId += 1; ... id: nextVariableId) is functionally identical to the pre-PR id: (nextSecretId += 1) — both pre-increment before assignment, so the first newly-added variable still gets id=41 instead of 40. The suggested id: nextVariableId++ (post-increment) would have actually fixed the off-by-one. Mock/storybook only — no production or test impact, just worth noting since the comment was resolved without the underlying issue being addressed.
Extended reasoning...
What's wrong
This file initializes nextVariableId = mockVariables.length + 1 = 40 at line 10611, since mockVariables contains 39 entries with ids 1..39. The CodeRabbit suggestion on this exact line was to change id: (nextSecretId += 1) to id: nextVariableId++ (post-increment), so the first newly-added variable would receive id=40 (no gap).
The rewrite in commit 9b1a12f split the increment onto its own line:
nextVariableId += 1;
mockVariables = [
...mockVariables,
{
name: variable.name,
id: nextVariableId,
...
},
];This is functionally identical to the original id: (nextSecretId += 1). Both pre-increment, then assign. So the off-by-one persists.
Step-by-step proof
mockVariablesis built with 39 entries vianew Array(39).fill(null).map((_, index) => ({ id: index + 1, ... }))— ids 1..39.nextVariableId = mockVariables.length + 1 = 40.- First call to
addVariable: line 10645 runsnextVariableId += 1→nextVariableIdbecomes41. - Line 10649 assigns
id: nextVariableId→ new variable getsid=41. id=40is never used. The CodeRabbit-suggested post-increment (nextVariableId++) would have used40first and incremented after, eliminating the gap.
Why this doesn't bite anything
- IDs remain unique (gap, not collision), so React keys and
parseInt(id, 10)filtering still work. - This is
frontend/services/mock_service/mocks/responses.ts— only used in the Storybook/local-dev MSW handlers, never production. - No tests assert on specific sequential id values.
Fix
- nextVariableId += 1;
mockVariables = [
...mockVariables,
{
name: variable.name,
- id: nextVariableId,
+ id: nextVariableId++,
created_at: new Date().toISOString(),
updated_at: new Date().toISOString(),
},
];Worth flagging only because the original CodeRabbit suggestion was explicitly marked "fixed" in a reply, but the rewrite kept the pre-increment semantics — so the resolved-comment trail doesn't reflect the actual code state. Severity: nit.
There was a problem hiding this comment.
ok, this was preexisitng and not breaking anything, will followup eventually
Issue
Closes #44326 4.86.0
Second half of #35483 from 4.85.0
Description
Screenshots of updates
Testing
Summary by CodeRabbit
New Features
Improvements
Style
Tests