Add Windows support for "Require all software" during setup experience.#44519
Add Windows support for "Require all software" during setup experience.#44519
Conversation
- Refactored `InstallSoftwareForm.tsx` to include Windows logic alongside macOS. - Added `require_all_software_windows` configuration to global settings, teams, and API. - Updated UI tests and components for platform-specific behavior. - Enhanced checkbox logic for macOS and Windows setup flows.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
/agentic_review |
Code Review by Qodo
1. Windows require-all not used
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #44519 +/- ##
=======================================
Coverage 66.79% 66.80%
=======================================
Files 2637 2637
Lines 212132 212151 +19
Branches 9555 9565 +10
=======================================
+ Hits 141690 141718 +28
+ Misses 57576 57567 -9
Partials 12866 12866
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:
|
|
Caution Review failedAn error occurred during the review process. Please try again later. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds Windows support for the Setup Experience "require all software" option: extends IMdmConfig and ITeam with Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/pages/ManageControlsPage/SetupExperience/cards/InstallSoftware/components/InstallSoftwareForm/InstallSoftwareForm.tests.tsx (1)
165-192: ⚡ Quick winAdd a save-path interaction test for Windows API invocation.
These assertions validate rendering, but not that Save actually triggers
mdmAPI.updateRequireAllSoftwareWindows(...)after checkbox interaction. Adding that test would catch regressions in the core behavior this PR introduces.Proposed test addition
+import mdmAPI from "services/entities/mdm"; ... +it("calls Windows require-all API on Save when checkbox is changed", async () => { + const updateRequireAllSoftwareWindowsSpy = jest + .spyOn(mdmAPI, "updateRequireAllSoftwareWindows") + .mockResolvedValue({}); + + const { user } = render( + <InstallSoftwareForm + savedRequireAllSoftwareWindows={false} + currentTeamId={1} + softwareTitles={[createMockSoftwareTitle()]} + hasManualAgentInstall={false} + platform="windows" + router={createMockRouter()} + refetchSoftwareTitles={noop} + /> + ); + + await user.click( + screen.getByRole("checkbox", { name: /Cancel setup if software fails/i }) + ); + await user.click(screen.getByRole("button", { name: "Save" })); + + await waitFor(() => { + expect(updateRequireAllSoftwareWindowsSpy).toHaveBeenCalledWith(1, true); + }); +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/pages/ManageControlsPage/SetupExperience/cards/InstallSoftware/components/InstallSoftwareForm/InstallSoftwareForm.tests.tsx` around lines 165 - 192, Add a test that verifies the checkbox interaction calls the Windows API: render InstallSoftwareForm with savedRequireAllSoftwareWindows set, mock mdmAPI.updateRequireAllSoftwareWindows, use screen.getByRole("checkbox", { name: /Cancel setup if software fails/ }) to click/toggle the checkbox (using userEvent or fireEvent) and await/assert that mdmAPI.updateRequireAllSoftwareWindows was called with the expected payload (teamId/currentTeamId and new boolean value). Ensure the test imports/mocks mdmAPI.updateRequireAllSoftwareWindows and uses the same platform="windows" and softwareTitles setup as the existing test so it only adds the interaction/assertion for updateRequireAllSoftwareWindows.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@frontend/pages/ManageControlsPage/SetupExperience/cards/InstallSoftware/components/InstallSoftwareForm/InstallSoftwareForm.tests.tsx`:
- Around line 165-192: Add a test that verifies the checkbox interaction calls
the Windows API: render InstallSoftwareForm with savedRequireAllSoftwareWindows
set, mock mdmAPI.updateRequireAllSoftwareWindows, use
screen.getByRole("checkbox", { name: /Cancel setup if software fails/ }) to
click/toggle the checkbox (using userEvent or fireEvent) and await/assert that
mdmAPI.updateRequireAllSoftwareWindows was called with the expected payload
(teamId/currentTeamId and new boolean value). Ensure the test imports/mocks
mdmAPI.updateRequireAllSoftwareWindows and uses the same platform="windows" and
softwareTitles setup as the existing test so it only adds the
interaction/assertion for updateRequireAllSoftwareWindows.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5cfff4d9-a5e8-4786-aa5a-7edfa82ec9ac
📒 Files selected for processing (7)
frontend/__mocks__/configMock.tsfrontend/interfaces/config.tsfrontend/interfaces/team.tsfrontend/pages/ManageControlsPage/SetupExperience/cards/InstallSoftware/InstallSoftware.tsxfrontend/pages/ManageControlsPage/SetupExperience/cards/InstallSoftware/components/InstallSoftwareForm/InstallSoftwareForm.tests.tsxfrontend/pages/ManageControlsPage/SetupExperience/cards/InstallSoftware/components/InstallSoftwareForm/InstallSoftwareForm.tsxfrontend/services/entities/mdm.ts
There was a problem hiding this comment.
Pull request overview
Adds Windows support for the “Require all software” (cancel setup on software failure) setting within the Setup Experience “Install software” flow, wiring the new config field through UI state, API calls, and typed config objects.
Changes:
- Added a new MDM service method to PATCH
require_all_software_windows. - Extended
InstallSoftwareFormto support Windows checkbox state + save behavior, and updated copy/tests. - Plumbed the new
require_all_software_windowsfield through config/team interfaces and mocks.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/services/entities/mdm.ts | Adds API method to update require_all_software_windows. |
| frontend/pages/ManageControlsPage/SetupExperience/cards/InstallSoftware/components/InstallSoftwareForm/InstallSoftwareForm.tsx | Adds Windows checkbox + state and routes save to the new API method. |
| frontend/pages/ManageControlsPage/SetupExperience/cards/InstallSoftware/components/InstallSoftwareForm/InstallSoftwareForm.tests.tsx | Updates copy expectations and adds a Windows render test for the checkbox. |
| frontend/pages/ManageControlsPage/SetupExperience/cards/InstallSoftware/InstallSoftware.tsx | Passes the saved Windows flag from global/team config down into the form. |
| frontend/interfaces/team.ts | Adds require_all_software_windows to the team MDM setup_experience type. |
| frontend/interfaces/config.ts | Adds require_all_software_windows to the global MDM setup_experience type. |
| frontend/mocks/configMock.ts | Updates mock config to include require_all_software_windows. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
- Included Windows logic in MDM configuration checks. - Updated UI to display platform-specific messaging for Windows MDM setup. - Added Windows-related test case for software setup validation.
- Updated MDM logic to exclude Windows from mandatory setup experience MDM requirements. - Added conditional checks to disable "Require all software" when Windows MDM is not configured. - Updated UI messaging for clarity in Windows-specific setup scenarios. - Introduced and validated test cases for Windows checkbox behavior and platform-specific configurations.
|
@coderabbitai full review |
|
/agentic_review |
✅ Actions performedFull review triggered. |
|
Persistent review updated to latest commit 906fa4e |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
frontend/services/entities/mdm.ts (1)
271-277: ⚡ Quick winPrefer the setup-experience endpoint constant for this new updater.
This new method is part of setup-experience writes, but it targets
MDM_SETUPwhile neighboring setup-experience mutations useMDM_SETUP_EXPERIENCE. Using the newer endpoint constant here improves consistency and avoids coupling new code to legacy route naming.Suggested diff
- updateRequireAllSoftwareWindows: (teamId: number, isEnabled: boolean) => { - const { MDM_SETUP } = endpoints; - return sendRequest("PATCH", MDM_SETUP, { + updateRequireAllSoftwareWindows: (teamId: number, isEnabled: boolean) => { + const { MDM_SETUP_EXPERIENCE } = endpoints; + return sendRequest("PATCH", MDM_SETUP_EXPERIENCE, { fleet_id: teamId, require_all_software_windows: isEnabled, }); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/services/entities/mdm.ts` around lines 271 - 277, The updateRequireAllSoftwareWindows function is using the legacy endpoint constant MDM_SETUP; change it to use MDM_SETUP_EXPERIENCE so the mutation aligns with other setup-experience writes—locate updateRequireAllSoftwareWindows in frontend/services/entities/mdm.ts and replace the MDM_SETUP reference with MDM_SETUP_EXPERIENCE in the sendRequest call while keeping the same HTTP method and payload.frontend/pages/ManageControlsPage/SetupExperience/cards/InstallSoftware/components/InstallSoftwareForm/InstallSoftwareForm.tsx (1)
79-92: ⚡ Quick winUse a safer default for
isWindowsMdmEnabled.Defaulting this prop to
truecan silently enable the Windows checkbox if a caller forgets to pass it. Afalsedefault is safer for enforcement gating.Suggested diff
- isWindowsMdmEnabled = true, + isWindowsMdmEnabled = false,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/pages/ManageControlsPage/SetupExperience/cards/InstallSoftware/components/InstallSoftwareForm/InstallSoftwareForm.tsx` around lines 79 - 92, The InstallSoftwareForm component currently defaults the prop isWindowsMdmEnabled to true which can accidentally enable the Windows MDM checkbox; change the default in the InstallSoftwareForm parameter destructuring to isWindowsMdmEnabled = false so the safer disabled state is used when callers omit the prop (keep the prop optional in the props type and ensure any logic using isWindowsMdmEnabled in InstallSoftwareForm and related render/validation branches continues to treat it as a boolean).
🤖 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/InstallSoftware/components/InstallSoftwareForm/InstallSoftwareForm.tests.tsx`:
- Around line 195-225: The test creates a spy updateRequireAllSoftwareWindowsSpy
that is only restored at the end of the test, which can leak if the test fails;
fix by ensuring the spy is always cleaned up—either wrap the spy usage in a
try/finally and call updateRequireAllSoftwareWindowsSpy.mockRestore() in
finally, or add a global cleanup like afterEach(() => jest.restoreAllMocks())
for this test file so updateRequireAllSoftwareWindows (and any other spies) are
reliably restored; locate the spy in the "calls the Windows require-all API on
Save when the Windows checkbox is toggled" test and apply one of these cleanup
approaches.
---
Nitpick comments:
In
`@frontend/pages/ManageControlsPage/SetupExperience/cards/InstallSoftware/components/InstallSoftwareForm/InstallSoftwareForm.tsx`:
- Around line 79-92: The InstallSoftwareForm component currently defaults the
prop isWindowsMdmEnabled to true which can accidentally enable the Windows MDM
checkbox; change the default in the InstallSoftwareForm parameter destructuring
to isWindowsMdmEnabled = false so the safer disabled state is used when callers
omit the prop (keep the prop optional in the props type and ensure any logic
using isWindowsMdmEnabled in InstallSoftwareForm and related render/validation
branches continues to treat it as a boolean).
In `@frontend/services/entities/mdm.ts`:
- Around line 271-277: The updateRequireAllSoftwareWindows function is using the
legacy endpoint constant MDM_SETUP; change it to use MDM_SETUP_EXPERIENCE so the
mutation aligns with other setup-experience writes—locate
updateRequireAllSoftwareWindows in frontend/services/entities/mdm.ts and replace
the MDM_SETUP reference with MDM_SETUP_EXPERIENCE in the sendRequest call while
keeping the same HTTP method and payload.
🪄 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: 7d30ad4a-fd23-44db-8a78-650ae2dfa015
📒 Files selected for processing (7)
frontend/__mocks__/configMock.tsfrontend/interfaces/config.tsfrontend/interfaces/team.tsfrontend/pages/ManageControlsPage/SetupExperience/cards/InstallSoftware/InstallSoftware.tsxfrontend/pages/ManageControlsPage/SetupExperience/cards/InstallSoftware/components/InstallSoftwareForm/InstallSoftwareForm.tests.tsxfrontend/pages/ManageControlsPage/SetupExperience/cards/InstallSoftware/components/InstallSoftwareForm/InstallSoftwareForm.tsxfrontend/services/entities/mdm.ts
There was a problem hiding this comment.
Pull request overview
Adds Windows parity for the Setup Experience “require all software” behavior (UI label: “Cancel setup if software fails”), wiring through frontend config types, UI state/save logic, and a new MDM API helper.
Changes:
- Added
updateRequireAllSoftwareWindowsto the frontend MDM service and wired it into the Install Software setup experience form. - Extended setup experience config/team interfaces and mocks to include
require_all_software_windows. - Updated/added UI tests to cover Windows rendering, disabled behavior, and save behavior.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| frontend/services/entities/mdm.ts | Adds Windows PATCH helper to update require_all_software_windows. |
| frontend/pages/ManageControlsPage/SetupExperience/cards/InstallSoftware/components/InstallSoftwareForm/InstallSoftwareForm.tsx | Adds Windows checkbox/state + save path + platform gating. |
| frontend/pages/ManageControlsPage/SetupExperience/cards/InstallSoftware/components/InstallSoftwareForm/InstallSoftwareForm.tests.tsx | Adds Windows-focused rendering/API/disabled tests and updates label text. |
| frontend/pages/ManageControlsPage/SetupExperience/cards/InstallSoftware/InstallSoftware.tsx | Plumbs saved Windows setting + Windows MDM enabled flag into the form. |
| frontend/interfaces/team.ts | Adds require_all_software_windows to team setup experience typing. |
| frontend/interfaces/config.ts | Adds require_all_software_windows to global config typing. |
| frontend/mocks/configMock.ts | Adds default mock value for require_all_software_windows. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@claude review once |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Flagged one correctness issue inline — the new Windows checkbox persists to the backend but the device-user setup-experience UI can't read require_all_software_windows because it isn't on the device API/types, so the failure flow never triggers on Windows hosts.
Extended reasoning...
Overview
This PR wires up a Windows tab and require_all_software_windows checkbox in the Setup Experience admin form, including: a new updateRequireAllSoftwareWindows service (now correctly using MDM_SETUP_EXPERIENCE rather than the deprecated apple-only endpoint), Windows-MDM-gated disabled state with appropriate tooltips, and supporting type/mock updates in IMdmConfig/ITeam/configMock.ts. Tests cover the new Windows checkbox visibility, the disabled state when Windows MDM is off, and that Save calls the new API.
Security risks
None observed. This is a UI/admin-config change touching frontend state and a single PATCH to an existing endpoint. No auth, crypto, or permission boundaries are modified.
Level of scrutiny
Moderate — small frontend surface, but it is the user-facing wiring for an end-to-end feature whose backend support landed in earlier PRs (#43011, #44285, #44061). Because the value of this PR is the full round-trip behavior on Windows, the read side matters as much as the write side.
Other factors
The inline finding is the most consequential issue on this PR: the admin UI lets you toggle and save require_all_software_windows, but DeviceGlobalMDMConfig (server/fleet/app.go) and IDeviceGlobalConfig.mdm (frontend/interfaces/config.ts) don't expose the Windows flag, and DeviceUserPage short-circuits the prop on isAppleHost. Net effect on Windows: the checkbox is functionally write-only and the failure flow never runs. Qodo and Copilot flagged the same issue. Other Copilot/Qodo nits in the timeline (deprecated endpoint, missing SCSS, Windows MDM gating, GitOps tooltip) all appear to have been addressed in 36181ad/906fa4e — only the device-side read path remains. Deferring to a human to confirm the device API extension is in scope for this PR vs. a follow-up.
| // 2. "require all software" update (macOS or Windows) | ||
| if (shouldUpdateRequireAll) { | ||
| try { | ||
| await mdmAPI.updateRequireAllSoftwareMacOS( | ||
| currentTeamId, | ||
| requireAllSoftwareMacOS | ||
| ); | ||
| if (platform === "windows") { | ||
| await mdmAPI.updateRequireAllSoftwareWindows( | ||
| currentTeamId, | ||
| requireAllSoftwareWindows | ||
| ); | ||
| } else { | ||
| await mdmAPI.updateRequireAllSoftwareMacOS( | ||
| currentTeamId, | ||
| requireAllSoftwareMacOS | ||
| ); | ||
| } | ||
| hadSuccess = true; | ||
| setTouchedRequireAll(false); | ||
| } catch (e) { |
There was a problem hiding this comment.
🔴 The new Windows checkbox in InstallSoftwareForm persists require_all_software_windows on the backend, but the device-user setup-experience UI never reads it: IDeviceGlobalConfig.mdm (frontend/interfaces/config.ts:103-107) and DeviceGlobalMDMConfig (server/fleet/app.go:1732-1735) only expose require_all_software_macos, and DeviceUserPage (frontend/pages/hosts/details/DeviceUserPage/DeviceUserPage.tsx:636-639) gates the value on isAppleHost, so for Windows hosts requireAllSoftware is always false. The new admin checkbox is therefore functionally write-only — fix by extending DeviceGlobalMDMConfig/IDeviceGlobalConfig with require_all_software_windows and updating DeviceUserPage to use it for Windows hosts.
Extended reasoning...
What the bug is
The PR adds an admin-side write path for require_all_software_windows (the new Windows checkbox in InstallSoftwareForm.tsx, plus mdmAPI.updateRequireAllSoftwareWindows), but it does not add the corresponding read path for the device-user setup experience. As a result, no matter how the admin sets the Windows checkbox, the device-user UI on a Windows host can never observe it, and the failure-on-software-install flow that is the whole point of this feature can never trigger for Windows.
The exact code path
frontend/interfaces/config.ts:103-107—IDeviceGlobalConfig.mdmonly declaresenabled_and_configuredandrequire_all_software_macos. No Windows field was added in this PR.server/fleet/app.go:1732-1735— the backendDeviceGlobalMDMConfig(the device-facing payload) likewise only hasRequireAllSoftware(json:require_all_software_macos). The Windows flag exists onAppConfigbut is never copied into the device payload.frontend/pages/hosts/details/DeviceUserPage/DeviceUserPage.tsx:636-639passes:For a Windows host,requireAllSoftware={ (isAppleHost && globalConfig?.mdm?.require_all_software_macos) ?? false }
isAppleHostisfalse, so the short-circuit always yieldsfalse.frontend/pages/hosts/details/DeviceUserPage/components/SettingUpYourDevice/SettingUpYourDevice.tsx:36:Withconst failedSoftware = requireAllSoftware ? getFailedSoftwareInstall(setupSteps) : null;
requireAllSoftware === false,failedSoftwareis alwaysnull, so the failure UI never renders.
Why existing code does not prevent it
Windows IS routed through SettingUpYourDevice — DeviceUserPage.tsx:356-359 includes host?.platform === "windows" in isSetupExperienceSoftwareEnabledPlatform. So the component renders; it just receives requireAllSoftware=false regardless of the saved setting. The PR also did not touch DeviceUserPage.tsx, IDeviceGlobalConfig, or DeviceGlobalMDMConfig, so the read side is unchanged from before this PR.
Step-by-step proof
- Admin visits Setup Experience → Install Software → Windows tab and checks "Cancel setup if software fails", then clicks Save.
InstallSoftwareFormcallsmdmAPI.updateRequireAllSoftwareWindows(teamId, true), which PATCHes/setup_experiencewith{ require_all_software_windows: true }. Backend persists this onAppConfig.MDM.MacOSSetup.RequireAllSoftwareWindows.- A Windows host enrolls. The device-user UI loads
/api/_version_/fleet/device/{token}/desktop(or equivalent) and gets back aDeviceGlobalConfigwhosemdmpayload is{ enabled_and_configured, require_all_software_macos }only — norequire_all_software_windowsfield is serialized (server/fleet/app.go:1732-1735). DeviceUserPageevaluatesrequireAllSoftware = (isAppleHost && globalConfig?.mdm?.require_all_software_macos) ?? false.isAppleHostisfalsefor Windows → expression evaluates tofalse ?? false→false.SettingUpYourDevicereceivesrequireAllSoftware={false}. Even if a software install fails,failedSoftwarestaysnull, so the user is never shown "Device setup failed" and setup is never canceled.
Net effect: the Windows checkbox is functionally write-only on the frontend, which directly contradicts the PR title "Add Windows support for Require all software during setup experience".
Impact
The core feature this PR claims to deliver does not work end-to-end on Windows. Admins can toggle the setting and the value will persist, but no Windows device will ever experience the failure flow it controls. Shipping this as-is would be misleading and likely require a follow-up bug report once a customer notices.
How to fix
- Backend: add
RequireAllSoftwareWindows booljson:"require_all_software_windows"`` toDeviceGlobalMDMConfigin `server/fleet/app.go` and populate it from the effective team/global config when building the device response (see `server/service/devices.go` around lines 206-229). - Frontend types: add
require_all_software_windows: boolean | nulltoIDeviceGlobalConfig.mdminfrontend/interfaces/config.ts. - Frontend UI: in
DeviceUserPage.tsx:636-639, change the prop to something like:requireAllSoftware={ ((isAppleHost && globalConfig?.mdm?.require_all_software_macos) || (host?.platform === "windows" && globalConfig?.mdm?.require_all_software_windows)) ?? false }
This was independently flagged by Qodo in the same PR review as "Windows require-all not used".
🔬 also observed by qodo
There was a problem hiding this comment.
The MDM BYOD setup experience will be handled in a separate PR/story.
…in `InstallSoftwareForm`.
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.
Added a checkbox for the setup experience page.
Related issue: Resolves #42854
Checklist for submitter
Testing
Summary by CodeRabbit
New Features
UI
Tests