Custom host vitals: Controls > Variables tab + secret variable created_at (#48555)#48634
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 44954-custom-host-vitals #48634 +/- ##
============================================================
+ Coverage 67.90% 67.97% +0.06%
============================================================
Files 3677 3685 +8
Lines 233665 233897 +232
Branches 12410 12459 +49
============================================================
+ Hits 158679 158990 +311
+ Misses 60722 60603 -119
- Partials 14264 14304 +40
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…ination + page size 20 (#48555)
…dal style stubs (#48555)
| }; | ||
| } | ||
|
|
||
| const Variables = ({ router, location }: IVariablesProps) => { |
There was a problem hiding this comment.
this was moved to GlobalVariables.tsx, and is now a Table instead of a PaginatedList (Figma: https://www.figma.com/design/kccHNGXUTi7e6A0L7WIfJh/-44954-Add-edit-delete-custom-host-vitals?node-id=5428-14629&t=3gpZX9MXKbrXMQjB-0)
Variables is now the container for GlobalVariablesTab + CustomHostVitalsTab.
| type SecretVariableIdentifier struct { | ||
| ID uint `json:"id" db:"id"` | ||
| Name string `json:"name" name:"name"` | ||
| CreatedAt string `json:"created_at" db:"created_at"` |
There was a problem hiding this comment.
Following Figma now showing Created as one of the columns for Global Variables.
| "name": "SOME_API_TOKEN", | ||
| "created_at": "2020-11-13T22:57:12Z", | ||
| "updated_at": "2020-11-13T22:57:12Z" |
There was a problem hiding this comment.
@rachaelshaw let me know if it's fine to keep this change in this PR or include it as part of #48369
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.
|
@coderabbitai full review |
✅ Action performedFull review finished. |
WalkthroughChangesThis PR restructures the Controls > Variables page into a sub-navigation shell with "Global variables" and "Custom host vitals" sections. Existing custom-variables logic is moved into a new Sequence Diagram(s)See diagrams embedded in the hidden review stack artifact for section routing, service data flow, tab data flow, and modal mutation flow. Possibly related PRs
Suggested labels: frontend, backend, enhancement Suggested reviewers: (none specified in provided context) Poem 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
frontend/services/entities/custom_host_vitals_mock.ts (2)
68-86: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueNo empty-name guard in mock
addCustomHostVital.The mock only rejects duplicates; an empty/whitespace-only
trimmedname is accepted and stored. If a caller's client-side validation doesn't strictly mirror this (e.g. validates the untrimmed value), a blank-named vital could slip through to the mock list. Worth double-checking againstAddCustomHostVitalModal's validation logic.🤖 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/entities/custom_host_vitals_mock.ts` around lines 68 - 86, The mock add flow in addCustomHostVital only checks for duplicate names and still accepts empty or whitespace-only values after trimming. Add the same empty-name validation used by AddCustomHostVitalModal before creating the ICustomHostVital, and reject blank inputs consistently so the mock cannot store a vital with an empty name.
48-66: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMock ignores pagination params.
getCustomHostVitalsnever appliesparams.page/params.per_page, andmeta.has_next_results/has_previous_resultsare hardcoded tofalse. Pagination controls inCustomHostVitalsTabwon't be exercised meaningfully against this mock until it's swapped for the real API.🤖 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/entities/custom_host_vitals_mock.ts` around lines 48 - 66, `getCustomHostVitals` in the custom host vitals mock currently ignores pagination and always returns all filtered items with `meta.has_next_results` and `meta.has_previous_results` set to false. Update the mock to honor `IListCustomHostVitalsApiParams.page` and `per_page` by slicing the filtered results accordingly, and compute the meta flags from the remaining items so `CustomHostVitalsTab` can exercise pagination behavior realistically.frontend/pages/ManageControlsPage/Variables/_styles.scss (1)
6-50: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated
__token/__actions/spinner styling between.global-variablesand.custom-host-vitals-tab.Consider extracting a shared SCSS mixin/placeholder for the common token/actions/spinner rules to avoid drift between the two card styles.
🤖 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/_styles.scss` around lines 6 - 50, The `.global-variables` and `.custom-host-vitals-tab` blocks repeat the same `__token`, `__actions`, and centered spinner styling, so refactor the shared rules into a reusable SCSS mixin or placeholder. Update the styles in `_styles.scss` to apply that shared abstraction from both card containers, keeping only the truly unique bits in each block so the token/actions layout stays consistent without duplication.frontend/pages/ManageControlsPage/Variables/components/EditCustomHostVitalModal/EditCustomHostVitalModal.tsx (1)
70-77: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider short-circuiting when the name is unchanged.
onClickSavealways callsupdateCustomHostVital()even ifname.trim() === vital.name, triggering an unnecessary write/round-trip.♻️ Optional short-circuit
const onClickSave = () => { const validation = validateFormData({ name }, true); if (!validation.isValid) { setFormValidation(validation); return; } + if (name.trim() === vital.name) { + onCancel(); + return; + } updateCustomHostVital(); };🤖 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/components/EditCustomHostVitalModal/EditCustomHostVitalModal.tsx` around lines 70 - 77, onClickSave in EditCustomHostVitalModal always triggers updateCustomHostVital even when the edited name is unchanged. Add a short-circuit after validateFormData so that, if name.trim() matches vital.name, the handler returns early and skips the update call; keep the existing validation and setFormValidation behavior intact.frontend/pages/ManageControlsPage/Variables/cards/CustomHostVitalsTab/CustomHostVitalsTab.tsx (1)
1-207: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftNo test coverage for the new CRUD tab.
Unlike
GlobalVariables.tests.tsxmentioned elsewhere in this stack, there's no test file forCustomHostVitalsTab. Given this drives add/edit/delete/search flows with permission and GitOps gating, tests would help lock in behavior once wired to the real API.🤖 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/cards/CustomHostVitalsTab/CustomHostVitalsTab.tsx` around lines 1 - 207, Add test coverage for CustomHostVitalsTab, since it now drives add/edit/delete/search behavior and permission/GitOps gating. Create a focused test file for the CustomHostVitalsTab component that exercises the key flows in renderContent, renderAddButton, and the modal handlers (onClickAdd, onSaveAdd, onSaveEdit, onDeleted). Mock AppContext, useGitOpsMode, and the customHostVitalsAPI query so you can verify admin vs non-admin behavior, GitOps-disabled button state, empty-state vs table rendering, search filtering, and that add/edit/delete actions trigger refetch and close the modals.
🤖 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.tsx`:
- Around line 33-44: The redirect guard in Variables.tsx only handles unknown
truthy sections, so the bare variables route never rewrites to the default URL.
Update the logic around currentSection and the router.replace call so the
component also redirects when section is undefined, using DEFAULT_SECTION.path
(and preserving location.search) in the Variables component. Keep the existing
unknown-section fallback behavior, but ensure both missing and invalid sections
land on DEFAULT_SECTION.urlSection.
In `@frontend/router/index.tsx`:
- Around line 344-345: The bare variables route is still rendering Variables
instead of redirecting to global-variables, so /controls/variables never reaches
the intended section. Update the routing in router/index.tsx so the variables
path explicitly redirects to /controls/variables/global-variables, while keeping
variables/:section mapped to Variables for sectioned URLs. Use the existing
Route definitions around Variables to locate and adjust the navigation behavior.
---
Nitpick comments:
In `@frontend/pages/ManageControlsPage/Variables/_styles.scss`:
- Around line 6-50: The `.global-variables` and `.custom-host-vitals-tab` blocks
repeat the same `__token`, `__actions`, and centered spinner styling, so
refactor the shared rules into a reusable SCSS mixin or placeholder. Update the
styles in `_styles.scss` to apply that shared abstraction from both card
containers, keeping only the truly unique bits in each block so the
token/actions layout stays consistent without duplication.
In
`@frontend/pages/ManageControlsPage/Variables/cards/CustomHostVitalsTab/CustomHostVitalsTab.tsx`:
- Around line 1-207: Add test coverage for CustomHostVitalsTab, since it now
drives add/edit/delete/search behavior and permission/GitOps gating. Create a
focused test file for the CustomHostVitalsTab component that exercises the key
flows in renderContent, renderAddButton, and the modal handlers (onClickAdd,
onSaveAdd, onSaveEdit, onDeleted). Mock AppContext, useGitOpsMode, and the
customHostVitalsAPI query so you can verify admin vs non-admin behavior,
GitOps-disabled button state, empty-state vs table rendering, search filtering,
and that add/edit/delete actions trigger refetch and close the modals.
In
`@frontend/pages/ManageControlsPage/Variables/components/EditCustomHostVitalModal/EditCustomHostVitalModal.tsx`:
- Around line 70-77: onClickSave in EditCustomHostVitalModal always triggers
updateCustomHostVital even when the edited name is unchanged. Add a
short-circuit after validateFormData so that, if name.trim() matches vital.name,
the handler returns early and skips the update call; keep the existing
validation and setFormValidation behavior intact.
In `@frontend/services/entities/custom_host_vitals_mock.ts`:
- Around line 68-86: The mock add flow in addCustomHostVital only checks for
duplicate names and still accepts empty or whitespace-only values after
trimming. Add the same empty-name validation used by AddCustomHostVitalModal
before creating the ICustomHostVital, and reject blank inputs consistently so
the mock cannot store a vital with an empty name.
- Around line 48-66: `getCustomHostVitals` in the custom host vitals mock
currently ignores pagination and always returns all filtered items with
`meta.has_next_results` and `meta.has_previous_results` set to false. Update the
mock to honor `IListCustomHostVitalsApiParams.page` and `per_page` by slicing
the filtered results accordingly, and compute the meta flags from the remaining
items so `CustomHostVitalsTab` can exercise pagination behavior realistically.
🪄 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: b9189019-b8d3-4fc8-ae6e-5f5065599659
⛔ Files ignored due to path filters (1)
docs/REST API/rest-api.mdis excluded by!**/*.md
📒 Files selected for processing (26)
frontend/interfaces/custom_host_vitals.tsfrontend/pages/ManageControlsPage/Variables/Variables.tsxfrontend/pages/ManageControlsPage/Variables/VariablesNavItems.tsxfrontend/pages/ManageControlsPage/Variables/_styles.scssfrontend/pages/ManageControlsPage/Variables/cards/CustomHostVitalsTab/CustomHostVitalsTab.tsxfrontend/pages/ManageControlsPage/Variables/cards/CustomHostVitalsTab/CustomHostVitalsTableConfig.tsxfrontend/pages/ManageControlsPage/Variables/cards/CustomHostVitalsTab/index.tsfrontend/pages/ManageControlsPage/Variables/cards/GlobalVariables/GlobalVariables.tests.tsxfrontend/pages/ManageControlsPage/Variables/cards/GlobalVariables/GlobalVariables.tsxfrontend/pages/ManageControlsPage/Variables/cards/GlobalVariables/GlobalVariablesTableConfig.tsxfrontend/pages/ManageControlsPage/Variables/cards/GlobalVariables/index.tsfrontend/pages/ManageControlsPage/Variables/components/AddCustomHostVitalModal/AddCustomHostVitalModal.tsxfrontend/pages/ManageControlsPage/Variables/components/AddCustomHostVitalModal/helpers.tsfrontend/pages/ManageControlsPage/Variables/components/AddCustomHostVitalModal/index.tsfrontend/pages/ManageControlsPage/Variables/components/DeleteCustomHostVitalModal/DeleteCustomHostVitalModal.tsxfrontend/pages/ManageControlsPage/Variables/components/DeleteCustomHostVitalModal/index.tsfrontend/pages/ManageControlsPage/Variables/components/EditCustomHostVitalModal/EditCustomHostVitalModal.tsxfrontend/pages/ManageControlsPage/Variables/components/EditCustomHostVitalModal/index.tsfrontend/router/index.tsxfrontend/router/paths.tsfrontend/services/entities/custom_host_vitals.tsfrontend/services/entities/custom_host_vitals_mock.tsfrontend/utilities/endpoints.tsserver/datastore/mysql/secret_variables.goserver/datastore/mysql/secret_variables_test.goserver/fleet/secret_variables.go
| const currentSection = | ||
| navItems.find((item) => item.urlSection === section) ?? DEFAULT_SECTION; | ||
|
|
||
| const onDeleteVariable = () => { | ||
| setShowDeleteModal(false); | ||
| refetch(); | ||
| }; | ||
| // Redirect unknown sections to the default section. | ||
| if ( | ||
| section && | ||
| currentSection === DEFAULT_SECTION && | ||
| section !== DEFAULT_SECTION.urlSection | ||
| ) { | ||
| router.replace(DEFAULT_SECTION.path.concat(location.search)); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Bare /controls/variables doesn't redirect to /controls/variables/global-variables.
The redirect guard only fires when section is truthy and unrecognized. When section is undefined (i.e., the user hits the bare variables route registered in router/index.tsx), currentSection silently falls back to DEFAULT_SECTION and the component renders that card in place — the URL is never rewritten to /controls/variables/global-variables, contradicting the stated acceptance criterion.
🔧 Proposed fix
// Redirect unknown sections to the default section.
- if (
- section &&
- currentSection === DEFAULT_SECTION &&
- section !== DEFAULT_SECTION.urlSection
- ) {
+ if (!section || (currentSection === DEFAULT_SECTION && section !== DEFAULT_SECTION.urlSection)) {
router.replace(DEFAULT_SECTION.path.concat(location.search));
return null;
}📝 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 currentSection = | |
| navItems.find((item) => item.urlSection === section) ?? DEFAULT_SECTION; | |
| const onDeleteVariable = () => { | |
| setShowDeleteModal(false); | |
| refetch(); | |
| }; | |
| // Redirect unknown sections to the default section. | |
| if ( | |
| section && | |
| currentSection === DEFAULT_SECTION && | |
| section !== DEFAULT_SECTION.urlSection | |
| ) { | |
| router.replace(DEFAULT_SECTION.path.concat(location.search)); | |
| return null; | |
| } | |
| const currentSection = | |
| navItems.find((item) => item.urlSection === section) ?? DEFAULT_SECTION; | |
| // Redirect unknown sections to the default section. | |
| if ( | |
| !section || | |
| (currentSection === DEFAULT_SECTION && | |
| section !== DEFAULT_SECTION.urlSection) | |
| ) { | |
| router.replace(DEFAULT_SECTION.path.concat(location.search)); | |
| return null; | |
| } |
🤖 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.tsx` around lines 33 -
44, The redirect guard in Variables.tsx only handles unknown truthy sections, so
the bare variables route never rewrites to the default URL. Update the logic
around currentSection and the router.replace call so the component also
redirects when section is undefined, using DEFAULT_SECTION.path (and preserving
location.search) in the Variables component. Keep the existing unknown-section
fallback behavior, but ensure both missing and invalid sections land on
DEFAULT_SECTION.urlSection.
| <Route path="variables" component={Variables} /> | ||
| <Route path="variables/:section" component={Variables} /> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Bare variables route never redirects to global-variables.
Per PR objectives, /controls/variables should redirect to /controls/variables/global-variables. With both variables and variables/:section routed to the same Variables component, the redirect entirely depends on client-side logic in Variables.tsx. As flagged there, that logic doesn't fire for the no-section case — see the Variables.tsx comment for details.
🤖 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/router/index.tsx` around lines 344 - 345, The bare variables route
is still rendering Variables instead of redirecting to global-variables, so
/controls/variables never reaches the intended section. Update the routing in
router/index.tsx so the variables path explicitly redirects to
/controls/variables/global-variables, while keeping variables/:section mapped to
Variables for sectioned URLs. Use the existing Route definitions around
Variables to locate and adjust the navigation behavior.
…ty for CreatedAt (testifylint) (#48555)
| @@ -361,4 +361,6 @@ export default { | |||
| }, | |||
| // custom variables endpoints | |||
| VARIABLES: `/${API_VERSION}/fleet/custom_variables`, | |||
There was a problem hiding this comment.
nit: Maybe we should rename this to GLOBAL_VARIABLES on a different PR to keep the language consistent?
| params: IListCustomHostVitalsApiParams | ||
| ): Promise<IListCustomHostVitalsResponse> { | ||
| const { CUSTOM_HOST_VITALS } = endpoints; | ||
| const path = `${CUSTOM_HOST_VITALS}?${buildQueryStringFromParams({ |
There was a problem hiding this comment.
This can be simplified by using the getPathWithQueryParams helper
| (currentSection === DEFAULT_SECTION && | ||
| section !== DEFAULT_SECTION.urlSection) | ||
| ) { | ||
| router.replace(DEFAULT_SECTION.path.concat(location.search)); |
There was a problem hiding this comment.
per our best practices, this needs to be inside an useEffect hook
| setShowDeleteModal(false); | ||
| refetch(); | ||
| }; | ||
| // Redirect the bare route (no section) and unknown sections to the default |
There was a problem hiding this comment.
Would it make sense to add a redirect directive at the router layer for variables -> global-variables? That would simplify things a little bit here, LMKYT
There was a problem hiding this comment.
I think either approach works, but I believe the codebase uses this useEffect pattern catch "bad" routes + preserve URL search parameters.
I did some quick testing by removing the useEffect and instead relying on a catch-all Route + IndexRoute, but:
- we'd need two
onEntercallbacks (repeated) to keep the URL search parameters - we'd need to remove the
:section- keeping
:sectionallows us extending the tabs easily in the future if we wanted to add another tab. - if we removed it, we'd have to add the tab and also declare it as a route
- keeping
What I tried:
<Route path="variables">
<IndexRoute
onEnter={(nextState, replace) =>
replace(
`${PATHS.CONTROLS_VARIABLES_GLOBAL_VARIABLES}${nextState.location.search}`
)
}
/>
<Route path="global-variables" component={Variables} />
<Route path="custom-host-vitals" component={Variables} />
<Route
path="*"
onEnter={(nextState, replace) =>
replace(
`${PATHS.CONTROLS_VARIABLES_GLOBAL_VARIABLES}${nextState.location.search}`
)
}
/>
</Route>There was a problem hiding this comment.
let me know if you have a strong preference, @juan-fdz-hawa -- but I'm tempted to keep the useEffect as I see it a little bit simpler and already centralized
| className={`${baseClass}__side-nav`} | ||
| navItems={navItems.map((navItem) => ({ | ||
| ...navItem, | ||
| path: navItem.path.concat(location.search), |
There was a problem hiding this comment.
nit: "${navItem.path}${location.search}"
| setShowAddModal(false); | ||
| refetch(); | ||
| }; | ||
| const currentSection = |
There was a problem hiding this comment.
Decomposing this will allow you to simplify the "redirect to default" guard bellow:
const defaultSection = navItems[0];
const matchedSection = navItems.find((item) => item.urlSection === section);
const currentSection = matchedSection ?? defaultSection;
...
useEffect(() => {
if (section && !matchedSection) {
router.replace(`${defaultSection.path}${location.search}`);
}
}, [section, matchedSection, defaultSection.path, location.search, router]);
| label="Name" | ||
| name="name" | ||
| error={formValidation.name?.message} | ||
| helpText="This will be the vital's label on the host detail page." |
There was a problem hiding this comment.
Should we add a max length constraint here? inputOptions={{ maxLength: NAME_MAX_LENGTH }}
| label="Name" | ||
| name="name" | ||
| error={formValidation.name?.message} | ||
| helpText="This will be the vital's label on the host detail page." |
There was a problem hiding this comment.
Should we add a max length constraint here? inputOptions={{ maxLength: NAME_MAX_LENGTH }}
| import { | ||
| validateFormData, | ||
| ICustomHostVitalFormValidation, | ||
| } from "../AddCustomHostVitalModal/helpers"; |
There was a problem hiding this comment.
I think you should move the helper to some other place for cleaner semantics, if the helper is going to be used by both the AddCustomHostVitalModal and EditCustomHostVitalModal components, then it should be in a place common to both.
There was a problem hiding this comment.
yeah honestly I kind of one-shotted this and didn't pay much attention to the final architecture, but I agree and will move this to maybe a helpers file one level up from where it's used
|
There are some gaps on the enforcement of GitOps mode. For Global variables, the in-page "Add variable" button is correctly disabled in GitOps mode, but there are ways to bypass this via either Command-palette entry or deep-link. Global variables are also deletable on GitOps mode (not sure if that should be changed here). |
|
You might want to update the command palette to include commands for the custom host vitals functionality added here - that could be done in a separate PR, up to you |
I tried not to change logic of Global variables -- just moving it to the new
Yeah I have a separate story for that 👍 (was thinking of including it here but don't want this to be a monster of a PR): #48686 |
Related issue: Resolves #48555
Custom host vitals (#44954) let admins define custom fields that surface on the host details page and can be referenced in scripts and configuration profiles. This PR builds the Controls > Variables frontend: a Global variables / Custom host vitals sub-nav, and the Custom host vitals CRUD tab (add/edit/delete, mock-backed until the API lands). It also exposes
created_aton the custom variables list endpoint so the table's Created column renders.Checklist for submitter
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Will add changes file in the feature branch directly.
Testing
Added/updated automated tests
QA'd all new/changed functionality manually
Screen.Recording.2026-07-02.at.1.52.45.PM.mov
Summary by CodeRabbit
New Features
Bug Fixes