UI housekeeping: Update Modal.children from JSX.Element to React.ReactNode, remove empty fragment wrappers - #41394
Merged
Merged
Conversation
jacobshandling
had a problem deploying
to
Docker Hub
March 10, 2026 20:07 — with 
GitHub Actions
Failure
GitHub Actions
Failure
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
WalkthroughThis pull request refactors modal component JSX structures across the frontend codebase by removing unnecessary React Fragment wrappers. The Modal component's Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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.
Actionable comments posted: 3
🧹 Nitpick comments (4)
frontend/pages/hosts/details/HostDetailsPage/modals/RecoveryLockPasswordModal/RecoveryLockPasswordModal.tsx (1)
52-75: LGTM! Clean refactoring of the conditional rendering.The logic correctly handles all three states: loading, error, and success. The Fragment on lines 59-73 is appropriately retained since it wraps multiple sibling elements.
One optional readability improvement: a chained ternary would make the mutual exclusivity of states explicit:
♻️ Optional: Chained ternary for clearer state handling
- {isLoading && <Spinner />} - {recoveryLockPasswordError ? ( + {isLoading ? ( + <Spinner /> + ) : recoveryLockPasswordError ? ( <DataError description={getErrorReason(recoveryLockPasswordError) || undefined} /> ) : ( - !isLoading && ( - <> - <InputFieldHiddenContent value={recoveryLockPassword ?? ""} /> - <p> - Use this to unlock and regain access to the host if the end user - forgets their local password.{" "} - <CustomLink - newTab - url={`${LEARN_MORE_ABOUT_BASE_LINK}/startup-security-macos`} - text="Learn more" - /> - </p> - <div className="modal-cta-wrap"> - <Button onClick={onCancel}>Done</Button> - </div> - </> - ) + <> + <InputFieldHiddenContent value={recoveryLockPassword ?? ""} /> + <p> + Use this to unlock and regain access to the host if the end user + forgets their local password.{" "} + <CustomLink + newTab + url={`${LEARN_MORE_ABOUT_BASE_LINK}/startup-security-macos`} + text="Learn more" + /> + </p> + <div className="modal-cta-wrap"> + <Button onClick={onCancel}>Done</Button> + </div> + </> )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/pages/hosts/details/HostDetailsPage/modals/RecoveryLockPasswordModal/RecoveryLockPasswordModal.tsx` around lines 52 - 75, Refactor the JSX conditional in RecoveryLockPasswordModal to use a chained ternary expression to make the three mutually-exclusive states explicit: check isLoading first to render Spinner, then check recoveryLockPasswordError to render DataError (using getErrorReason(recoveryLockPasswordError) as description), otherwise render the success block containing InputFieldHiddenContent (value={recoveryLockPassword ?? ""}), the explanatory paragraph with CustomLink and the Done Button (onClick={onCancel}); update references to Spinner, DataError, InputFieldHiddenContent, CustomLink and Button in the JSX so the mutual exclusivity of states is clear and equivalent to the current behavior.frontend/pages/admin/IntegrationsPage/cards/MdmSettings/VppPage/components/EditTeamsVppModal/EditTeamsVppModal.tsx (1)
283-286: Optional: Fragment wrapper could be removed for consistency with PR objectives.The
tooltipprop acceptsReact.ReactNode, so the fragment wrapper around the text content is unnecessary.🔧 Proposed simplification
tooltip={ isDropdownDisabled ? undefined : ( - <> - Each fleet can have only one VPP token. Fleets that already - have a VPP token won't show up here. - </> + "Each fleet can have only one VPP token. Fleets that already have a VPP token won't show up here." ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/pages/admin/IntegrationsPage/cards/MdmSettings/VppPage/components/EditTeamsVppModal/EditTeamsVppModal.tsx` around lines 283 - 286, The tooltip prop in EditTeamsVppModal is receiving an unnecessary React fragment around plain text; remove the fragment wrapper so the tooltip prop receives the text node directly (e.g., replace the <>Each fleet... </> fragment with the raw string or a single JSX text node) in the EditTeamsVppModal component where tooltip is passed.frontend/pages/ManageControlsPage/Scripts/components/CancelScriptBatchModal/CancelScriptBatchModal.tsx (1)
28-51: Consider removing the wrapper div to reduce DOM nesting.The Modal component wraps children in
<div className="modal__content">. This component further wraps content in<div className="cancel-script-batch-modal__content">, creating unnecessary double-wrapping. Since no styles target thecancel-script-batch-modal__contentclass, the inner wrapper can be removed—children can be rendered directly as Modal's direct children.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/pages/ManageControlsPage/Scripts/components/CancelScriptBatchModal/CancelScriptBatchModal.tsx` around lines 28 - 51, The inner wrapper div with class `${baseClass}__content` inside the CancelScriptBatchModal component is redundant because Modal already renders a wrapper; remove that div and render its children (paragraphs and the `.modal-cta-wrap` block with the Buttons) directly as Modal's children, keeping all existing elements, props (isLoading, disabled, onClick handlers) and the `baseClass` constant for any remaining per-component classes if needed; ensure there are no remaining references to `cancel-script-batch-modal__content` and run the app to verify styling/behavior unchanged.frontend/pages/hosts/details/HostDetailsPage/modals/RunScriptModal/RunScriptModal.tsx (1)
78-80: MissingsetPagein dependency array.While
setPagefromuseStateis typically stable across renders, ESLint'sexhaustive-depsrule would flag this. Adding it to the dependency array follows React best practices.Suggested fix
const onQueryChange = useCallback(({ pageIndex }: ITableQueryData) => { setPage(pageIndex); - }, []); + }, [setPage]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/pages/hosts/details/HostDetailsPage/modals/RunScriptModal/RunScriptModal.tsx` around lines 78 - 80, The onQueryChange callback created with useCallback omits setPage from its dependency array, which will trigger exhaustive-deps lint warnings; update the declaration of onQueryChange to include setPage in the dependency array so useCallback depends on setPage (i.e., useCallback(({ pageIndex }: ITableQueryData) => { setPage(pageIndex); }, [setPage])) ensuring the callback list references the stable state setter used inside it.
🤖 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/hosts/details/cards/Software/SelfService/components/UninstallSoftwareModal/UninstallSoftwareModal.tsx`:
- Around line 51-54: The confirmation copy uses the optional prop softwareName
which can be undefined; change the paragraph to use the fallback value
displaySoftwareName (the same value used in the modal title) instead of
softwareName so the body never renders "undefined data" and remains consistent
with the title in UninstallSoftwareModal (referencing the softwareName and
displaySoftwareName variables).
In
`@frontend/pages/hosts/details/DeviceUserPage/BitLockerPinModal/BitLockerPinModal.tsx`:
- Around line 25-58: The JSX in BitLockerPinModal (BitLockerPinModal.tsx) wraps
an <ol> inside a <p>, which is invalid; remove the enclosing <p> that contains
the <ol> and instead render the <ol> directly (or wrap the list in a suitable
block-level container like a <div> if additional grouping or styling is needed)
so the ordered list is not nested inside a paragraph and spacing/styling remain
correct.
In
`@frontend/pages/hosts/ManageHostsPage/components/RunScriptBatchModal/RunScriptBatchModal.tsx`:
- Around line 364-372: The "Cancel" Button currently calls
setSelectedScript(undefined) but does not dismiss the modal via onCancel, so
either rename the button to reflect it returns to the previous step (e.g.,
"Back" or "Previous") or change its handler to call onCancel to actually close
the modal; locate the Button in RunScriptBatchModal.tsx that uses
setSelectedScript and update its label and/or onClick to call the modal-dismiss
handler onCancel (or both if you want to clear selectedScript then close).
---
Nitpick comments:
In
`@frontend/pages/admin/IntegrationsPage/cards/MdmSettings/VppPage/components/EditTeamsVppModal/EditTeamsVppModal.tsx`:
- Around line 283-286: The tooltip prop in EditTeamsVppModal is receiving an
unnecessary React fragment around plain text; remove the fragment wrapper so the
tooltip prop receives the text node directly (e.g., replace the <>Each fleet...
</> fragment with the raw string or a single JSX text node) in the
EditTeamsVppModal component where tooltip is passed.
In
`@frontend/pages/hosts/details/HostDetailsPage/modals/RecoveryLockPasswordModal/RecoveryLockPasswordModal.tsx`:
- Around line 52-75: Refactor the JSX conditional in RecoveryLockPasswordModal
to use a chained ternary expression to make the three mutually-exclusive states
explicit: check isLoading first to render Spinner, then check
recoveryLockPasswordError to render DataError (using
getErrorReason(recoveryLockPasswordError) as description), otherwise render the
success block containing InputFieldHiddenContent (value={recoveryLockPassword ??
""}), the explanatory paragraph with CustomLink and the Done Button
(onClick={onCancel}); update references to Spinner, DataError,
InputFieldHiddenContent, CustomLink and Button in the JSX so the mutual
exclusivity of states is clear and equivalent to the current behavior.
In
`@frontend/pages/hosts/details/HostDetailsPage/modals/RunScriptModal/RunScriptModal.tsx`:
- Around line 78-80: The onQueryChange callback created with useCallback omits
setPage from its dependency array, which will trigger exhaustive-deps lint
warnings; update the declaration of onQueryChange to include setPage in the
dependency array so useCallback depends on setPage (i.e., useCallback(({
pageIndex }: ITableQueryData) => { setPage(pageIndex); }, [setPage])) ensuring
the callback list references the stable state setter used inside it.
In
`@frontend/pages/ManageControlsPage/Scripts/components/CancelScriptBatchModal/CancelScriptBatchModal.tsx`:
- Around line 28-51: The inner wrapper div with class `${baseClass}__content`
inside the CancelScriptBatchModal component is redundant because Modal already
renders a wrapper; remove that div and render its children (paragraphs and the
`.modal-cta-wrap` block with the Buttons) directly as Modal's children, keeping
all existing elements, props (isLoading, disabled, onClick handlers) and the
`baseClass` constant for any remaining per-component classes if needed; ensure
there are no remaining references to `cancel-script-batch-modal__content` and
run the app to verify styling/behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 57c7cde1-cbee-4ef7-b29e-c7d56496cfb2
📒 Files selected for processing (75)
frontend/components/ActivityDetails/InstallDetails/SoftwareInstallDetailsModal/SoftwareInstallDetailsModal.tsxfrontend/components/ActivityDetails/InstallDetails/SoftwareIpaInstallDetailsModal/SoftwareIpaInstallDetailsModal.tsxfrontend/components/ActivityDetails/InstallDetails/SoftwareScriptDetailsModal/SoftwareScriptDetailsModal.tsxfrontend/components/ActivityDetails/InstallDetails/SoftwareUninstallDetailsModal/SoftwareUninstallDetailsModal.tsxfrontend/components/ActivityDetails/InstallDetails/VppInstallDetailsModal/VppInstallDetailsModal.tsxfrontend/components/Modal/Modal.tsxfrontend/pages/AccountPage/AccountPage.tsxfrontend/pages/DashboardPage/cards/ActivityFeed/components/AppStoreDetailsModal/AppStoreDetailsModal.tsxfrontend/pages/DashboardPage/cards/ActivityFeed/components/LibrarySoftwareDetailsModal/LibrarySoftwareDetailsModal.tsxfrontend/pages/DashboardPage/cards/ActivityFeed/components/RunScriptDetailsModal/RunScriptDetailsModal.tsxfrontend/pages/DashboardPage/components/MdmSolutionModal/MdmSolutionModal.tsxfrontend/pages/ManageControlsPage/OSSettings/cards/Certificates/components/DeleteCertificateModal/DeleteCertificateModal.tsxfrontend/pages/ManageControlsPage/OSSettings/cards/CustomSettings/components/ConfigProfileStatusModal/ConfigProfileStatusModal.tsxfrontend/pages/ManageControlsPage/OSSettings/cards/CustomSettings/components/ProfileUploader/components/AddProfileModal/AddProfileModal.tsxfrontend/pages/ManageControlsPage/OSSettings/cards/CustomSettings/components/ResendConfigProfileModal/ResendConfigProfileModal.tsxfrontend/pages/ManageControlsPage/OSSettings/cards/Passwords/Passwords.tsxfrontend/pages/ManageControlsPage/Scripts/components/CancelScriptBatchModal/CancelScriptBatchModal.tsxfrontend/pages/ManageControlsPage/Scripts/components/EditScriptModal/EditScriptModal.tsxfrontend/pages/ManageControlsPage/Scripts/components/ScriptUploadModal/ScriptUploadModal.tsxfrontend/pages/ManageControlsPage/Secrets/components/DeleteSecretModal/DeleteSecretModal.tsxfrontend/pages/ManageControlsPage/SetupExperience/cards/RunScript/components/DeleteSetupExperienceScriptModal/DeleteSetupExperienceScriptModal.tsxfrontend/pages/ManageControlsPage/SetupExperience/cards/SetupAssistant/components/DeleteAutoEnrollmentProfile/DeleteAutoEnrollmentProfile.tsxfrontend/pages/SoftwarePage/SoftwareAddPage/SoftwareFleetMaintained/FleetMaintainedAppDetailsPage/AddFleetAppSoftwareModal/AddFleetAppSoftwareModal.tsxfrontend/pages/SoftwarePage/SoftwareAddPage/SoftwareFleetMaintained/FleetMaintainedAppDetailsPage/FleetAppDetailsModal/FleetAppDetailsModal.tsxfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/AdvancedOptionsModal/AdvancedOptionsModal.tsxfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/DeleteSoftwareModal/DeleteSoftwareModal.tsxfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/EditAutoUpdateConfigModal/EditAutoUpdateConfigModal.tsxfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/EditConfigurationModal/EditConfigurationModal.tsxfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/EditIconModal/EditIconModal.tsxfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/ViewYamlModal/ViewYamlModal.tsxfrontend/pages/SoftwarePage/components/modals/CategoriesEndUserExperienceModal/CategoriesEndUserExperienceModal.tsxfrontend/pages/SoftwarePage/components/modals/PreviewTicketModal/PreviewTicketModal.tsxfrontend/pages/admin/IntegrationsPage/cards/CertificateAuthorities/components/AddCertAuthorityModal/AddCertAuthorityModal.tsxfrontend/pages/admin/IntegrationsPage/cards/CertificateAuthorities/components/DeleteCertificateAuthorityModal/DeleteCertificateAuthorityModal.tsxfrontend/pages/admin/IntegrationsPage/cards/ConditionalAccess/ConditionalAccess.tsxfrontend/pages/admin/IntegrationsPage/cards/ConditionalAccess/components/EntraConditionalAccessModal/EntraConditionalAccessModal.tsxfrontend/pages/admin/IntegrationsPage/cards/ConditionalAccess/components/OktaConditionalAccessModal/OktaConditionalAccessModal.tsxfrontend/pages/admin/IntegrationsPage/cards/MdmSettings/AndroidMdmPage/components/TurnOffAndroidMdmModal/TurnOffAndroidMdmModal.tsxfrontend/pages/admin/IntegrationsPage/cards/MdmSettings/AppleBusinessManagerPage/components/AddAbmModal/AddAbmModal.tsxfrontend/pages/admin/IntegrationsPage/cards/MdmSettings/AppleBusinessManagerPage/components/DeleteAbmModal/DeleteAbmModal.tsxfrontend/pages/admin/IntegrationsPage/cards/MdmSettings/AppleBusinessManagerPage/components/EditTeamsAbmModal/EditTeamsAbmModal.tsxfrontend/pages/admin/IntegrationsPage/cards/MdmSettings/VppPage/components/AddVppModal/AddVppModal.tsxfrontend/pages/admin/IntegrationsPage/cards/MdmSettings/VppPage/components/DeleteVppModal/DeleteVppModal.tsxfrontend/pages/admin/IntegrationsPage/cards/MdmSettings/VppPage/components/EditTeamsVppModal/EditTeamsVppModal.tsxfrontend/pages/admin/IntegrationsPage/cards/MdmSettings/VppPage/components/RenewVppModal/RenewVppModal.tsxfrontend/pages/admin/IntegrationsPage/cards/MdmSettings/components/AddEntraTenantModal/AddEntraTenantModal.tsxfrontend/pages/admin/IntegrationsPage/cards/MdmSettings/components/DeleteEntraTenantModal/DeleteEntraTenantModal.tsxfrontend/pages/admin/IntegrationsPage/cards/MdmSettings/components/ExampleWebhookUrlPayloadModal/ExampleWebhookUrlPayloadModal.tsxfrontend/pages/admin/TeamManagementPage/TeamDetailsWrapper/UsersPage/components/RemoveUserModal/RemoveUserModal.tsxfrontend/pages/admin/TeamManagementPage/components/DeleteTeamModal/DeleteTeamModal.tsxfrontend/pages/admin/components/HostStatusWebhookPreviewModal/HostStatusWebhookPreviewModal.tsxfrontend/pages/hosts/ManageHostsPage/components/DeleteLabelModal/DeleteLabelModal.tsxfrontend/pages/hosts/ManageHostsPage/components/RunScriptBatchModal/RunScriptBatchModal.tsxfrontend/pages/hosts/components/CommandDetailsModal/CommandDetailsModal.tsxfrontend/pages/hosts/components/DeleteHostModal/DeleteHostModal.tsxfrontend/pages/hosts/components/ScriptDetailsModal/ScriptDetailsModal.tsxfrontend/pages/hosts/components/TransferHostModal/TransferHostModal.tsxfrontend/pages/hosts/details/DeviceUserPage/BitLockerPinModal/BitLockerPinModal.tsxfrontend/pages/hosts/details/DeviceUserPage/BypassModal/BypassModal.tsxfrontend/pages/hosts/details/HostDetailsPage/modals/BootstrapPackageModal/BootstrapPackageModal.tsxfrontend/pages/hosts/details/HostDetailsPage/modals/CancelActivityModal/CancelActivityModal.tsxfrontend/pages/hosts/details/HostDetailsPage/modals/LockModal/LockModal.tsxfrontend/pages/hosts/details/HostDetailsPage/modals/RecoveryLockPasswordModal/RecoveryLockPasswordModal.tsxfrontend/pages/hosts/details/HostDetailsPage/modals/RunScriptModal/RunScriptModal.tsxfrontend/pages/hosts/details/HostDetailsPage/modals/SelectQueryModal/SelectQueryModal.tsxfrontend/pages/hosts/details/HostDetailsPage/modals/UnlockModal/UnlockModal.tsxfrontend/pages/hosts/details/HostDetailsPage/modals/WipeModal/WipeModal.tsxfrontend/pages/hosts/details/OSSettingsModal/OSSettingsModal.tsxfrontend/pages/hosts/details/cards/Software/SelfService/components/OpenSoftwareModal/OpenSoftwareModal.tsxfrontend/pages/hosts/details/cards/Software/SelfService/components/SoftwareUpdateModal/SoftwareUpdateModal.tsxfrontend/pages/hosts/details/cards/Software/SelfService/components/UninstallSoftwareModal/UninstallSoftwareModal.tsxfrontend/pages/hosts/details/modals/CertificateDetailsModal/CertificateDetailsModal.tsxfrontend/pages/hosts/details/modals/InventoryVersionsModal/InventoryVersionsModal.tsxfrontend/pages/hosts/details/modals/LocationModal/LocationModal.tsxfrontend/pages/policies/ManagePoliciesPage/components/CalendarEventPreviewModal/CalendarEventPreviewModal.tsx
💤 Files with no reviewable changes (1)
- frontend/pages/ManageControlsPage/OSSettings/cards/Passwords/Passwords.tsx
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #41394 +/- ##
==========================================
- Coverage 66.35% 66.35% -0.01%
==========================================
Files 2480 2480
Lines 198570 198569 -1
Branches 8772 8885 +113
==========================================
- Hits 131765 131764 -1
Misses 54904 54904
Partials 11901 11901
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:
|
jacobshandling
marked this pull request as draft
March 10, 2026 21:05
jacobshandling
temporarily deployed
to
Docker Hub
March 10, 2026 21:17 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
jacobshandling
temporarily deployed
to
Docker Hub
March 10, 2026 21:59 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
jacobshandling
marked this pull request as ready for review
March 10, 2026 22:04
getvictor
reviewed
Mar 10, 2026
| </li> | ||
| <li> | ||
| <span>5.</span> | ||
| <span>Enter a name for the server such as "Fleet".</span> |
Member
There was a problem hiding this comment.
Looks like there are other changes here besides the ReactNode stuff
Contributor
Author
There was a problem hiding this comment.
An initial pass with Sonnet made some counterproductive substitutions of "s for smart quotes which broke a couple things. I ran a couple follow-up passes with Opus 4.6 to address that, and this was something it flagged. I wasn't sure whether or not this was a change the initial pass had made, so went ahead with this, though now see that it is an update relative to main
getvictor
approved these changes
Mar 10, 2026
getvictor
left a comment
getvictor
left a comment
Member
There was a problem hiding this comment.
LGTM
There were some additional whitespace, wrapping, quoting changes here. Would have been nice to not have them in this large PR.
Contributor
Author
@getvictor these were a byproduct of the reduced nesting that removing the wrapper fragments generated, since lines that previously were too long to fit into one now fit on a single line. Linting would not pass without them. I commented on the quote changes here |
This was referenced Apr 10, 2026
This was referenced Apr 20, 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
2 participants
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.
Summary by CodeRabbit