Fleet UI: Custom variables page updates - #43810
Merged
Merged
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #43810 +/- ##
==========================================
- Coverage 66.90% 66.90% -0.01%
==========================================
Files 2600 2600
Lines 208391 208406 +15
Branches 9333 9225 -108
==========================================
+ Hits 139428 139437 +9
- Misses 56275 56281 +6
Partials 12688 12688
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[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.
Contributor
There was a problem hiding this comment.
Pull request overview
Updates the Controls > Variables view to align with the styling/layout patterns used across other Controls pages and renames/moves components to match “custom variables” UI terminology.
Changes:
- Swaps the
/controls/variablesroute to use the newVariablespage component and updates related component/module names. - Updates the Variables page layout to use
PageDescription, adjusts empty/loading states, and refactors header/actions placement. - Replaces legacy “description”
<p>blocks in several Controls pages with the sharedPageDescriptioncomponent and removes now-unused SCSS selectors.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/router/index.tsx | Routes /controls/variables to the new Variables component. |
| frontend/pages/ManageControlsPage/Variables/index.ts | Adds barrel export for the Variables page. |
| frontend/pages/ManageControlsPage/Variables/Variables.tsx | Renames/restructures Variables page layout; adds query-param modal open behavior. |
| frontend/pages/ManageControlsPage/Variables/Variables.tests.tsx | Updates tests to reference Variables and new empty-state copy. |
| frontend/pages/ManageControlsPage/Variables/_styles.scss | Renames base class to .variables and aligns modal/form styles with new class names. |
| frontend/pages/ManageControlsPage/Variables/components/AddCustomVariableModal/index.ts | Adds barrel export for the add modal. |
| frontend/pages/ManageControlsPage/Variables/components/AddCustomVariableModal/AddCustomVariableModal.tsx | Renames add modal component and class names for “custom variables” terminology. |
| frontend/pages/ManageControlsPage/Variables/components/AddCustomVariableModal/helpers.ts | Renames validation types to match add custom variable modal. |
| frontend/pages/ManageControlsPage/Variables/components/DeleteCustomVariableModal/index.ts | Adds barrel export for the delete modal. |
| frontend/pages/ManageControlsPage/Variables/components/DeleteCustomVariableModal/DeleteCustomVariableModal.tsx | Renames delete modal component and class names for “custom variables”. |
| frontend/pages/ManageControlsPage/Variables/components/DeleteCustomVariableModal/_styles.scss | Renames delete modal base CSS class. |
| frontend/pages/ManageControlsPage/SetupExperience/SetupExperience.tsx | Uses PageDescription instead of a <p> description. |
| frontend/pages/ManageControlsPage/SetupExperience/_styles.scss | Removes unused __description styling. |
| frontend/pages/ManageControlsPage/Scripts/Scripts.tsx | Uses PageDescription instead of a <p> description. |
| frontend/pages/ManageControlsPage/Scripts/_styles.scss | Removes unused __description styling. |
| frontend/pages/ManageControlsPage/OSUpdates/OSUpdates.tsx | Uses PageDescription instead of a <p> description. |
| frontend/pages/ManageControlsPage/OSUpdates/_styles.scss | Removes unused __description styling. |
| frontend/pages/ManageControlsPage/OSSettings/OSSettings.tsx | Uses PageDescription instead of a <p> description. |
| frontend/pages/ManageControlsPage/OSSettings/_styles.scss | Removes unused __description styling. |
| frontend/pages/ManageControlsPage/Secrets/index.ts | Removes legacy barrel export (directory removed/renamed). |
| frontend/pages/ManageControlsPage/Secrets/components/AddSecretModal/index.ts | Removes legacy barrel export (directory removed/renamed). |
| frontend/pages/ManageControlsPage/Secrets/components/DeleteSecretModal/index.ts | Removes legacy barrel export (directory removed/renamed). |
Comments suppressed due to low confidence (5)
frontend/pages/ManageControlsPage/Variables/Variables.tsx:65
- The
add_variablequery-param logic only runs once on mount (empty dependency array) and reads fromwindow.location.search. If navigation updates only the search string while keeping this route component mounted (common with react-router), the modal won't open. Consider using the route-injectedlocation.searchand re-running the effect when it changes, and userouter.replace(or equivalent) instead ofwindow.history.replaceStateto remove the param.
frontend/pages/ManageControlsPage/Variables/Variables.tsx:50 - This file has been renamed to "Variables", but several exported/public identifiers still use "SECRETS" terminology (e.g.,
SECRETS_PAGE_SIZEand the react-query key"secrets"). Renaming these to match the page/domain vocabulary will make debugging (React Query devtools, logs) and maintenance easier and aligns with the PR goal of updating UI terminology.
frontend/pages/ManageControlsPage/Variables/components/AddCustomVariableModal/AddCustomVariableModal.tsx:28 - Although this component is now "AddCustomVariableModal", some user-facing messages within it still say "secret" (e.g., the 409 conflict message and the generic save error). Please update those strings to consistently use "custom variable"/"variable" terminology to match the renamed UI.
frontend/pages/ManageControlsPage/Variables/components/DeleteCustomVariableModal/DeleteCustomVariableModal.tsx:23 - In the modal body text, the string "This will delete the" is immediately followed by a
<b>element without an explicit space, which will render as "delete the". Please add an explicit space before the<b>so the sentence reads correctly.
frontend/pages/ManageControlsPage/Variables/Variables.tsx:65 - New behavior is introduced to open the "Add custom variable" modal via the
?add_variable=1query param, but there’s no test covering it inVariables.tests.tsx. Adding a test that sets the URL/search param and asserts the modal opens (and the param is removed) would help prevent regressions.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
WalkthroughThis PR refactors the Controls > Variables page to follow Fleet UI patterns. It replaces inline description 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}
Contributor
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/components/AddCustomVariableModal/AddCustomVariableModal.tsx (1)
67-77:⚠️ Potential issue | 🟡 MinorUser-facing error messages still say "secret" instead of "variable".
Since this PR renames the feature to "custom variable" everywhere (modal title, success flash on line 67, delete modal text), these two error flashes look like they were missed. They'll confuse users who see "Variable created." on success but "A secret with this name already exists." on conflict. Note
DeleteCustomVariableModal.tsxhas already been updated to say "custom variable" in its error path.✏️ Proposed wording fix
} catch (error: any) { if (error.status === 409) { - renderFlash("error", "A secret with this name already exists."); + renderFlash( + "error", + "A custom variable with this name already exists." + ); } else { renderFlash( "error", - "An error occurred while saving the secret. Please try again." + "An error occurred while saving the custom variable. Please try again." ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/pages/ManageControlsPage/Variables/components/AddCustomVariableModal/AddCustomVariableModal.tsx` around lines 67 - 77, Update the two error flash messages in AddCustomVariableModal.tsx to use the new feature name ("custom variable") instead of "secret": replace "A secret with this name already exists." with "A custom variable with this name already exists." (the conflict error branch) and replace "An error occurred while saving the secret. Please try again." with "An error occurred while saving the custom variable. Please try again." (the generic error branch); these use renderFlash inside the try/catch that also calls onSave and renders the success message.
🧹 Nitpick comments (3)
frontend/pages/ManageControlsPage/Variables/_styles.scss (1)
35-35: Consider finishing the terminology rename for the copy icon class.
variables__copy-secret-iconstill carries the old “secret” naming. If there is no compatibility reason to keep it, rename this selector and its matching JSX class together.♻️ Possible selector rename
- .variables__copy-secret-icon { + .variables__copy-variable-icon {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/pages/ManageControlsPage/Variables/_styles.scss` at line 35, The selector .variables__copy-secret-icon still uses the old "secret" terminology; rename this selector to the new term (e.g., .variables__copy-icon) and update the matching JSX/className usage wherever variables__copy-secret-icon is referenced (search for variables__copy-secret-icon in components/JSX and in the stylesheet), ensuring consistency between the SCSS selector and the component className and updating any tests or storybook snapshots that reference the old class.frontend/pages/ManageControlsPage/Variables/components/AddCustomVariableModal/helpers.ts (1)
13-13:Omit<IAddCustomVariableFormData, "isValid">is a no-op here.
IAddCustomVariableFormDatais{ name: string; value: string }and has noisValidproperty, so theOmitdoesn't exclude anything. This is leftover from the oldIAddSecretModalScheduleFormDatashape. Safe to simplify.-type IFormValidationKey = keyof Omit<IAddCustomVariableFormData, "isValid">; +type IFormValidationKey = keyof IAddCustomVariableFormData;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/pages/ManageControlsPage/Variables/components/AddCustomVariableModal/helpers.ts` at line 13, IFormValidationKey currently uses Omit<IAddCustomVariableFormData, "isValid"> but IAddCustomVariableFormData has no isValid property, so the Omit is a no-op and should be removed; update the type alias IFormValidationKey to use keyof IAddCustomVariableFormData directly (replace the Omit usage) so the type is simplified and reflects the actual shape.frontend/pages/ManageControlsPage/Variables/Variables.tsx (1)
52-65: Consider using the router API to stripadd_variableinstead ofwindow.history.replaceState.The codebase convention is to use the router API (
router.push(),router.replace()) for navigation and query param manipulation rather than directly mutatingwindow.history. This keeps routing state synchronized across the application. If you prefer to keep this lightweight without adding a router dependency, that's acceptable — just wanted to flag the pattern difference.The effect's empty dependency array is correct for this use case (runs once on mount for a route-level component).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/pages/ManageControlsPage/Variables/Variables.tsx` around lines 52 - 65, The effect currently reads URLSearchParams and manually calls window.history.replaceState to remove the add_variable param; replace that manual history mutation with the router API: import and call useRouter(), use router.replace(...) (or router.push with replace behavior) to set the same pathname and the cleaned query (delete "add_variable") and include shallow: true to avoid a full navigation; keep the existing logic that calls setShowAddModal(true) when params.get("add_variable") === "1" and only change the history update to use router.replace so routing state stays synchronized (update inside the same useEffect that currently uses useEffect, referencing useEffect and setShowAddModal).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@frontend/pages/ManageControlsPage/Variables/components/AddCustomVariableModal/AddCustomVariableModal.tsx`:
- Around line 67-77: Update the two error flash messages in
AddCustomVariableModal.tsx to use the new feature name ("custom variable")
instead of "secret": replace "A secret with this name already exists." with "A
custom variable with this name already exists." (the conflict error branch) and
replace "An error occurred while saving the secret. Please try again." with "An
error occurred while saving the custom variable. Please try again." (the generic
error branch); these use renderFlash inside the try/catch that also calls onSave
and renders the success message.
---
Nitpick comments:
In `@frontend/pages/ManageControlsPage/Variables/_styles.scss`:
- Line 35: The selector .variables__copy-secret-icon still uses the old "secret"
terminology; rename this selector to the new term (e.g., .variables__copy-icon)
and update the matching JSX/className usage wherever variables__copy-secret-icon
is referenced (search for variables__copy-secret-icon in components/JSX and in
the stylesheet), ensuring consistency between the SCSS selector and the
component className and updating any tests or storybook snapshots that reference
the old class.
In
`@frontend/pages/ManageControlsPage/Variables/components/AddCustomVariableModal/helpers.ts`:
- Line 13: IFormValidationKey currently uses Omit<IAddCustomVariableFormData,
"isValid"> but IAddCustomVariableFormData has no isValid property, so the Omit
is a no-op and should be removed; update the type alias IFormValidationKey to
use keyof IAddCustomVariableFormData directly (replace the Omit usage) so the
type is simplified and reflects the actual shape.
In `@frontend/pages/ManageControlsPage/Variables/Variables.tsx`:
- Around line 52-65: The effect currently reads URLSearchParams and manually
calls window.history.replaceState to remove the add_variable param; replace that
manual history mutation with the router API: import and call useRouter(), use
router.replace(...) (or router.push with replace behavior) to set the same
pathname and the cleaned query (delete "add_variable") and include shallow: true
to avoid a full navigation; keep the existing logic that calls
setShowAddModal(true) when params.get("add_variable") === "1" and only change
the history update to use router.replace so routing state stays synchronized
(update inside the same useEffect that currently uses useEffect, referencing
useEffect and setShowAddModal).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3ef95f3b-fd8c-47d7-8ada-8b69a278cc42
📒 Files selected for processing (22)
frontend/pages/ManageControlsPage/OSSettings/OSSettings.tsxfrontend/pages/ManageControlsPage/OSSettings/_styles.scssfrontend/pages/ManageControlsPage/OSUpdates/OSUpdates.tsxfrontend/pages/ManageControlsPage/OSUpdates/_styles.scssfrontend/pages/ManageControlsPage/Scripts/Scripts.tsxfrontend/pages/ManageControlsPage/Scripts/_styles.scssfrontend/pages/ManageControlsPage/Secrets/components/AddSecretModal/index.tsfrontend/pages/ManageControlsPage/Secrets/components/DeleteSecretModal/index.tsfrontend/pages/ManageControlsPage/Secrets/index.tsfrontend/pages/ManageControlsPage/SetupExperience/SetupExperience.tsxfrontend/pages/ManageControlsPage/SetupExperience/_styles.scssfrontend/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/AddCustomVariableModal/helpers.tsfrontend/pages/ManageControlsPage/Variables/components/AddCustomVariableModal/index.tsfrontend/pages/ManageControlsPage/Variables/components/DeleteCustomVariableModal/DeleteCustomVariableModal.tsxfrontend/pages/ManageControlsPage/Variables/components/DeleteCustomVariableModal/_styles.scssfrontend/pages/ManageControlsPage/Variables/components/DeleteCustomVariableModal/index.tsfrontend/pages/ManageControlsPage/Variables/index.tsfrontend/router/index.tsx
💤 Files with no reviewable changes (7)
- frontend/pages/ManageControlsPage/OSSettings/_styles.scss
- frontend/pages/ManageControlsPage/SetupExperience/_styles.scss
- frontend/pages/ManageControlsPage/Secrets/index.ts
- frontend/pages/ManageControlsPage/Secrets/components/AddSecretModal/index.ts
- frontend/pages/ManageControlsPage/Secrets/components/DeleteSecretModal/index.ts
- frontend/pages/ManageControlsPage/OSUpdates/_styles.scss
- frontend/pages/ManageControlsPage/Scripts/_styles.scss
cdcme
approved these changes
Apr 21, 2026
3 tasks
This was referenced May 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue
Closes #36661
Description
Screenshots
Testing
Summary by CodeRabbit
New Features
Refactor