Fleet UI: Users page disable checkboxes not meeting requirements - #44080
Conversation
|
Are there disabled tooltips like this elsewhere? What are those copies like? Searched for 5 patterns (ctrl+o to expand) ⏺ Here's the pattern across Fleet: Setup experience cards (empty state messages, not tooltips):
Integration settings (info text / empty states):
Calendar / policies:
The consistent pattern is "To [action], first [turn on / enable / connect] [thing]." — using "turn on" for MDM toggles and "enable" / |
| }; | ||
| macos_setup: { | ||
| enable_managed_local_account?: boolean; | ||
| enable_create_local_admin_account?: boolean; |
There was a problem hiding this comment.
Can you double check these keys. The form is working properly but I thought I changed them in my last PR for a reason
There was a problem hiding this comment.
@RachelElysia This is correct! 🥇 it matches the renameto tag on the backend.
In #43353, we changed setup_experience.enable_managed_local_account to macos_setup.enable_managed_local_account because setup_experience uses the renameto names, not the json tag names. So in that case, we had the wrong field name (enable_managed_local_account) in the right section (setup_experience). Here, we have the right field name in the right section 👍
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #44080 +/- ##
==========================================
- Coverage 66.75% 66.74% -0.02%
==========================================
Files 2623 2628 +5
Lines 211048 211369 +321
Branches 9387 9380 -7
==========================================
+ Hits 140886 141070 +184
- Misses 57343 57477 +134
- Partials 12819 12822 +3
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.
There was a problem hiding this comment.
Pull request overview
Updates Fleet’s UI to prevent users from interacting with Setup experience “Users” checkboxes when prerequisite configuration is missing (IdP for end user auth; Apple MDM for managed local account), and adjusts managed-account action visibility logic in host actions.
Changes:
- Disable “End user authentication” (and “Lock end user info”) when IdP isn’t configured; add tooltip guidance.
- Disable “Managed local account” when Apple MDM isn’t enabled/configured; add tooltip guidance.
- Allow “Show managed account” host action to appear when a host already has a managed account status, even if the global setting is currently off; update types/tests to support new config key naming.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| frontend/pages/hosts/details/HostDetailsPage/HostActionsDropdown/helpers.tsx | Tweaks eligibility logic for showing the “Show managed account” action based on existing per-host status. |
| frontend/pages/ManageControlsPage/SetupExperience/cards/Users/components/UsersForm/UsersForm.tsx | Adds prerequisite-based disabled states + tooltips for the two checkboxes and wires submit handling through the form. |
| frontend/pages/ManageControlsPage/SetupExperience/cards/Users/components/UsersForm/UsersForm.tests.tsx | Adds test coverage for the new disabled states and non-toggle behavior. |
| frontend/pages/ManageControlsPage/SetupExperience/cards/Users/Users.tsx | Always renders the form and passes isIdPConfigured down instead of gating the entire card. |
| frontend/interfaces/team.ts | Extends MDM setup experience typings for the renamed managed-local-account flag; makes macos_setup optional. |
| frontend/interfaces/config.ts | Extends global MDM config typings for the renamed managed-local-account flag; makes macos_setup optional. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import SectionHeader from "components/SectionHeader/SectionHeader"; | ||
| import Spinner from "components/Spinner"; | ||
| import GenericMsgWithNavButton from "components/GenericMsgWithNavButton"; | ||
|
|
||
| import CustomLink from "components/CustomLink"; |
There was a problem hiding this comment.
PATHS is no longer referenced anywhere in this file after removing the IdP gating component. Please remove the unused import PATHS from "router/paths"; to avoid lint/TS unused-import failures.
WalkthroughThe PR updates the MDM configuration interface to introduce an optional Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/pages/ManageControlsPage/SetupExperience/cards/Users/components/UsersForm/UsersForm.tsx (1)
136-160:⚠️ Potential issue | 🟡 MinorMissing tooltip on disabled "Lock end user info".
When
isIdPConfiguredis false butisEndUserAuthEnabledis true (e.g., the setting was saved previously and the IdP was subsequently unconfigured), this checkbox renders disabled with no tooltip explaining why. Consider wrapping it in the same IdP-gatedTooltipWrapperused for "End user authentication" so users get a consistent explanation. The existing explanatory tooltip on the label can coexist viadisableTooltipgating.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/pages/ManageControlsPage/SetupExperience/cards/Users/components/UsersForm/UsersForm.tsx` around lines 136 - 160, The disabled "Lock end user info" checkbox lacks an explanatory tooltip when isEndUserAuthEnabled is true but isIdPConfigured is false; wrap the Checkbox (the element using props value={lockEndUserInfo} and onChange={onChangeLockEndUserInfo}) with the same TooltipWrapper pattern used for the "End user authentication" control and supply a tipContent explaining that IdP must be configured to enable this option, while preserving the existing inner TooltipWrapper on the label by conditionally passing a disableTooltip (or similar gating prop) when the outer IdP tooltip is shown; keep the Checkbox disabled via disabled={gitOpsModeEnabled || !isIdPConfigured}.frontend/pages/ManageControlsPage/SetupExperience/cards/Users/Users.tsx (1)
22-42:⚠️ Potential issue | 🟠 MajorHostDetailsPage still reads the legacy path for managed local account flag.
Users.tsxreadsmdm.setup_experience.enable_create_local_admin_account, butHostDetailsPage.tsx(line 1037) still reads the legacy pathmdm.macos_setup.enable_managed_local_account. Additionally,UsersForm.tsxsendsenable_managed_local_accountto the API. If the backend only populates the newsetup_experiencepath in responses,HostDetailsPagewill always computeisManagedLocalAccountEnabled=false, causing the "Show managed account" option to be incorrectly gated for hosts without an existingmanagedAccountStatus.Update
HostDetailsPage.tsxline 1037 to read frommdm.setup_experience.enable_create_local_admin_accountto match the rest of the PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/pages/ManageControlsPage/SetupExperience/cards/Users/Users.tsx` around lines 22 - 42, HostDetailsPage is still reading the legacy flag mdm.macos_setup.enable_managed_local_account; change the lookup at the isManagedLocalAccountEnabled computation (the code around HostDetailsPage's logic that currently reads mdm.macos_setup.enable_managed_local_account) to read mdm.setup_experience.enable_create_local_admin_account instead so it matches Users.tsx and UsersForm behavior and prevents isManagedLocalAccountEnabled from always being false when only the new path is populated.
🧹 Nitpick comments (2)
frontend/pages/ManageControlsPage/SetupExperience/cards/Users/Users.tsx (1)
83-89: Nit:isIdPConfiguredparam type usesPickbut consumer passes fullmdm.
isIdPConfiguredacceptsPick<IMdmConfig, "end_user_authentication">, which is fine since a fullIMdmConfigsatisfies it. Just note that the check assumesidpis always defined (non-optional onIMdmConfig). If the backend ever starts returning a missingend_user_authenticationblock (unlikely given current typing), this would throw onidp.entity_id. Consider optional chaining for defensive safety, or leave as-is if the backend contract is firm.🛡️ Optional defensive tweak
-const isIdPConfigured = ({ - end_user_authentication: idp, -}: Pick<IMdmConfig, "end_user_authentication">) => { - return ( - !!idp.entity_id && !!idp.idp_name && (!!idp.metadata_url || !!idp.metadata) - ); -}; +const isIdPConfigured = ({ + end_user_authentication: idp, +}: Pick<IMdmConfig, "end_user_authentication">) => { + return ( + !!idp?.entity_id && + !!idp?.idp_name && + (!!idp?.metadata_url || !!idp?.metadata) + ); +};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/pages/ManageControlsPage/SetupExperience/cards/Users/Users.tsx` around lines 83 - 89, isIdPConfigured currently destructures end_user_authentication as idp and assumes it is always present, which could throw if end_user_authentication becomes undefined; update the function (isIdPConfigured) to handle a possibly undefined idp by using optional chaining (or adjust the parameter type to allow undefined and guard it) so checks become safe (e.g., idp?.entity_id, idp?.idp_name, idp?.metadata_url || idp?.metadata) and return false when idp is missing.frontend/pages/ManageControlsPage/SetupExperience/cards/Users/components/UsersForm/UsersForm.tests.tsx (1)
130-166: Optional: strengthen no-toggle assertions.These tests start with the checkbox unchecked (via
defaultProps) and assert it remains unchecked after a click. The assertions are valid, but if theCheckboxever changed its handling ofaria-disabledclicks, a toggle would make the box checked and the test would catch it. Consider also asserting the opposite direction to more explicitly pin the behavior — e.g., render once withdefaultEnableManagedLocalAccount/defaultIsEndUserAuthEnabledtrue and verify the click also doesn't toggle it off.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/pages/ManageControlsPage/SetupExperience/cards/Users/components/UsersForm/UsersForm.tests.tsx` around lines 130 - 166, Update the tests to assert non-toggle behavior in both directions by adding mirror cases that start with the checkbox checked and verify a click does not uncheck it: modify the tests for "End user authentication" and "Managed local account" (in UsersForm.tests.tsx) to render UsersForm with defaultIsEndUserAuthEnabled true and defaultEnableManagedLocalAccount true respectively (use render or renderWithMdmEnabled as appropriate) and after simulating a user.click on the checkbox assert it remains checked; keep the existing tests that start unchecked to ensure both directions are pinned.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@frontend/pages/ManageControlsPage/SetupExperience/cards/Users/components/UsersForm/UsersForm.tsx`:
- Around line 161-204: The nested TooltipWrapper around the "Managed local
account" label can overlap with the outer TooltipWrapper when Apple MDM is not
configured; update the inner TooltipWrapper inside the Checkbox so it is
disabled when !isMacMdmEnabledAndConfigured (e.g., pass disableTooltip={
!isMacMdmEnabledAndConfigured }), or alternatively merge the inner tipContent
into the outer TooltipWrapper's tipContent when isMacMdmEnabledAndConfigured is
false so only one tooltip is rendered; target the TooltipWrapper that wraps the
label text (inside the Checkbox) and keep existing props like
enableManagedLocalAccount and onToggleManagedLocalAccount unchanged.
---
Outside diff comments:
In
`@frontend/pages/ManageControlsPage/SetupExperience/cards/Users/components/UsersForm/UsersForm.tsx`:
- Around line 136-160: The disabled "Lock end user info" checkbox lacks an
explanatory tooltip when isEndUserAuthEnabled is true but isIdPConfigured is
false; wrap the Checkbox (the element using props value={lockEndUserInfo} and
onChange={onChangeLockEndUserInfo}) with the same TooltipWrapper pattern used
for the "End user authentication" control and supply a tipContent explaining
that IdP must be configured to enable this option, while preserving the existing
inner TooltipWrapper on the label by conditionally passing a disableTooltip (or
similar gating prop) when the outer IdP tooltip is shown; keep the Checkbox
disabled via disabled={gitOpsModeEnabled || !isIdPConfigured}.
In `@frontend/pages/ManageControlsPage/SetupExperience/cards/Users/Users.tsx`:
- Around line 22-42: HostDetailsPage is still reading the legacy flag
mdm.macos_setup.enable_managed_local_account; change the lookup at the
isManagedLocalAccountEnabled computation (the code around HostDetailsPage's
logic that currently reads mdm.macos_setup.enable_managed_local_account) to read
mdm.setup_experience.enable_create_local_admin_account instead so it matches
Users.tsx and UsersForm behavior and prevents isManagedLocalAccountEnabled from
always being false when only the new path is populated.
---
Nitpick comments:
In
`@frontend/pages/ManageControlsPage/SetupExperience/cards/Users/components/UsersForm/UsersForm.tests.tsx`:
- Around line 130-166: Update the tests to assert non-toggle behavior in both
directions by adding mirror cases that start with the checkbox checked and
verify a click does not uncheck it: modify the tests for "End user
authentication" and "Managed local account" (in UsersForm.tests.tsx) to render
UsersForm with defaultIsEndUserAuthEnabled true and
defaultEnableManagedLocalAccount true respectively (use render or
renderWithMdmEnabled as appropriate) and after simulating a user.click on the
checkbox assert it remains checked; keep the existing tests that start unchecked
to ensure both directions are pinned.
In `@frontend/pages/ManageControlsPage/SetupExperience/cards/Users/Users.tsx`:
- Around line 83-89: isIdPConfigured currently destructures
end_user_authentication as idp and assumes it is always present, which could
throw if end_user_authentication becomes undefined; update the function
(isIdPConfigured) to handle a possibly undefined idp by using optional chaining
(or adjust the parameter type to allow undefined and guard it) so checks become
safe (e.g., idp?.entity_id, idp?.idp_name, idp?.metadata_url || idp?.metadata)
and return false when idp is missing.
🪄 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: 52fb6170-d93c-4300-bb4f-1e9aa016f208
📒 Files selected for processing (6)
frontend/interfaces/config.tsfrontend/interfaces/team.tsfrontend/pages/ManageControlsPage/SetupExperience/cards/Users/Users.tsxfrontend/pages/ManageControlsPage/SetupExperience/cards/Users/components/UsersForm/UsersForm.tests.tsxfrontend/pages/ManageControlsPage/SetupExperience/cards/Users/components/UsersForm/UsersForm.tsxfrontend/pages/hosts/details/HostDetailsPage/HostActionsDropdown/helpers.tsx
| <TooltipWrapper | ||
| tipContent={ | ||
| !isMacMdmEnabledAndConfigured ? ( | ||
| <span> | ||
| To enable, first turn on{" "} | ||
| <CustomLink | ||
| url={PATHS.ADMIN_INTEGRATIONS_MDM_APPLE} | ||
| text="Apple MDM" | ||
| variant="tooltip-link" | ||
| /> | ||
| . | ||
| </span> | ||
| ) : undefined | ||
| } | ||
| disableTooltip={!!isMacMdmEnabledAndConfigured} | ||
| underline={false} | ||
| position="left" | ||
| showArrow | ||
| > | ||
| <TooltipWrapper tipContent="Creates a hidden managed local admin account for remote troubleshooting on macOS hosts."> | ||
| Managed local account | ||
| </TooltipWrapper> | ||
| </Checkbox> | ||
| <Checkbox | ||
| disabled={gitOpsModeEnabled || !isMacMdmEnabledAndConfigured} | ||
| value={enableManagedLocalAccount} | ||
| onChange={onToggleManagedLocalAccount} | ||
| helpText={ | ||
| <span> | ||
| Fleet generates a user (_fleetadmin) and unique password for | ||
| each host, accessible in <b>Host details</b> >{" "} | ||
| <b>Show managed account</b>. | ||
| </span> | ||
| } | ||
| > | ||
| <TooltipWrapper | ||
| tipContent={ | ||
| <> | ||
| Creates a hidden managed local admin account for | ||
| <br /> | ||
| remote troubleshooting on macOS hosts. | ||
| </> | ||
| } | ||
| > | ||
| Managed local account | ||
| </TooltipWrapper> | ||
| </Checkbox> | ||
| </TooltipWrapper> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check TooltipWrapper implementation and whether nested usage is expected/handled.
fd -t f 'TooltipWrapper.tsx' frontend/components
echo '---'
rg -nP 'disableTooltip|hoverRef|onMouseEnter' --type=tsx -C2 -g '**/TooltipWrapper/**'Repository: fleetdm/fleet
Length of output: 339
🏁 Script executed:
cat frontend/components/TooltipWrapper/TooltipWrapper.tsxRepository: fleetdm/fleet
Length of output: 5529
🏁 Script executed:
rg 'disableTooltip' --type ts -B2 -A2 frontend/components/TooltipWrapperRepository: fleetdm/fleet
Length of output: 2460
🏁 Script executed:
rg -n 'TooltipWrapper.*TooltipWrapper' --type ts -A5 frontend/Repository: fleetdm/fleet
Length of output: 50370
🏁 Script executed:
# Look for nested TooltipWrapper patterns
rg -n '<TooltipWrapper' --type ts -A20 | grep -B5 -A15 'TooltipWrapper.*TooltipWrapper' | head -100Repository: fleetdm/fleet
Length of output: 9910
🏁 Script executed:
# Look at the actual UsersForm file to see the full context
sed -n '161,204p' frontend/pages/ManageControlsPage/SetupExperience/cards/Users/components/UsersForm/UsersForm.tsxRepository: fleetdm/fleet
Length of output: 1482
Verify nested tooltip behavior on "Managed local account".
When Apple MDM is not configured, the outer TooltipWrapper shows "To enable, first turn on Apple MDM", while the inner TooltipWrapper around the label text always displays "Creates a hidden managed local admin account...". Both tooltips can render simultaneously on hover, potentially causing overlapping popovers. Consider either (a) disabling the inner tooltip when !isMacMdmEnabledAndConfigured, or (b) merging both messages into a single tooltip state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@frontend/pages/ManageControlsPage/SetupExperience/cards/Users/components/UsersForm/UsersForm.tsx`
around lines 161 - 204, The nested TooltipWrapper around the "Managed local
account" label can overlap with the outer TooltipWrapper when Apple MDM is not
configured; update the inner TooltipWrapper inside the Checkbox so it is
disabled when !isMacMdmEnabledAndConfigured (e.g., pass disableTooltip={
!isMacMdmEnabledAndConfigured }), or alternatively merge the inner tipContent
into the outer TooltipWrapper's tipContent when isMacMdmEnabledAndConfigured is
false so only one tooltip is rendered; target the TooltipWrapper that wraps the
label text (inside the Checkbox) and keep existing props like
enableManagedLocalAccount and onToggleManagedLocalAccount unchanged.
Issue
Closes #42946
Description
Screenshots
Screen.Recording.2026-04-23.at.4.11.49.PM.mov
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Testing
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests