simplify OS modal - #43252
Merged
Merged
Conversation
![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.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #43252 +/- ##
==========================================
- Coverage 66.84% 66.84% -0.01%
==========================================
Files 2587 2588 +1
Lines 207336 207333 -3
Branches 9207 9318 +111
==========================================
- Hits 138591 138588 -3
- Misses 56135 56136 +1
+ Partials 12610 12609 -1
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:
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR simplifies the Host details OS settings modal UI by removing the dedicated error/details presentation and consolidating information into the status tooltip, while keeping resend/rotate actions in a compact action column.
Changes:
- Removes “(pending)” from several status labels and enables clickable tooltips for better UX.
- Moves error details into the Status tooltip for failed profiles, and replaces the “Details” column content with an actions-only (resend/rotate) cell.
- Updates styling and test suite to reflect the new table layout and behaviors.
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/pages/ManageControlsPage/OSSettings/cards/DiskEncryption/components/DiskEncryptionTable/DiskEncryptionTableConfig.tsx | Removes “(pending)” from disk encryption status labels in the Controls table. |
| frontend/pages/hosts/details/OSSettingsModal/OSSettingsTable/OSSettingStatusCell/OSSettingStatusCell.tsx | Adds failed-status error tooltip support and makes tooltips clickable; adjusts pending label text for Android cert flow. |
| frontend/pages/hosts/details/OSSettingsModal/OSSettingsTable/OSSettingStatusCell/OSSettingStatusCell.tests.tsx | Updates expectations for new status label text (removing “(pending)” in covered cases). |
| frontend/pages/hosts/details/OSSettingsModal/OSSettingsTable/OSSettingStatusCell/helpers.ts | Updates status label text for install/remove flows (but pending text remains in some configs). |
| frontend/pages/hosts/details/OSSettingsModal/OSSettingsTable/OSSettingStatusCell/errorTooltipHelpers.tsx | Refactors prior error-cell logic into a tooltip content generator for failed status. |
| frontend/pages/hosts/details/OSSettingsModal/OSSettingsTable/OSSettingsTableConfig.tsx | Replaces the “Details” column with a blank-header actions column; passes full profile to status cell. |
| frontend/pages/hosts/details/OSSettingsModal/OSSettingsTable/OSSettingsResendCell/OSSettingsResendCell.tsx | New actions-only cell for resend/rotate behavior. |
| frontend/pages/hosts/details/OSSettingsModal/OSSettingsTable/OSSettingsResendCell/OSSettingsResendCell.tests.tsx | New unit tests for resend/rotate button visibility rules. |
| frontend/pages/hosts/details/OSSettingsModal/OSSettingsTable/OSSettingsResendCell/index.ts | Barrel export for the new actions cell component. |
| frontend/pages/hosts/details/OSSettingsModal/OSSettingsTable/OSSettingsResendCell/_styles.scss | Styles actions cell as right-aligned, button-only content. |
| frontend/pages/hosts/details/OSSettingsModal/OSSettingsTable/OSSettingsNameCell/_styles.scss | Removes name cell tooltip sizing overrides (layout adjustment). |
| frontend/pages/hosts/details/OSSettingsModal/OSSettingsTable/OSSettingsErrorCell/OSSettingsErrorCell.tests.tsx | Removes tests for the deleted Error cell behavior. |
| frontend/pages/hosts/details/OSSettingsModal/OSSettingsTable/OSSettingsErrorCell/index.ts | Removes barrel export for deleted Error cell. |
| frontend/pages/hosts/details/OSSettingsModal/OSSettingsTable/_styles.scss | Adjusts table column sizing/overflow and updates hover rules for new resend/rotate classes. |
| changes/40702-simplif-os-modal | Adds changelog entry for the OS settings modal layout improvement. |
Comments suppressed due to low confidence (1)
frontend/pages/hosts/details/OSSettingsModal/OSSettingsTable/OSSettingStatusCell/errorTooltipHelpers.tsx:165
- The doc comment still refers to generating a tooltip for the “error column”, but this helper is now used to populate the tooltip content for the status cell when status is failed. Updating the wording would prevent confusion for future maintainers.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
WalkthroughThe PR simplifies the OS settings modal interface by removing the "(pending)" suffix from pending status labels across disk encryption and MDM profile display configurations. It consolidates error handling by removing the Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/pages/hosts/details/OSSettingsModal/OSSettingsTable/OSSettingStatusCell/helpers.ts (1)
111-196:⚠️ Potential issue | 🟠 MajorPending suffix removal is incomplete across status configs.
Line 136, Line 142, Line 172, Line 188, and Line 193 still render
"(pending)", so users will see inconsistent status text depending on row type/platform.Suggested patch
export const WINDOWS_DISK_ENCRYPTION_DISPLAY_CONFIG: WindowsDiskEncryptionDisplayConfig = { @@ pending: { - statusText: "Enforcing (pending)", + statusText: "Enforcing", @@ action_required: { - statusText: "Action required (pending)", + statusText: "Action required", @@ export const LINUX_DISK_ENCRYPTION_DISPLAY_CONFIG: LinuxDiskEncryptionDisplayConfig = { @@ action_required: { - statusText: "Action required (pending)", + statusText: "Action required", @@ export const RECOVERY_LOCK_PASSWORD_DISPLAY_CONFIG: Record< RecoveryLockPasswordStatus, ProfileDisplayOption > = { @@ pending: { - statusText: "Enforcing (pending)", + statusText: "Enforcing", @@ removing_enforcement: { - statusText: "Removing enforcement (pending)", + statusText: "Removing enforcement",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/pages/hosts/details/OSSettingsModal/OSSettingsTable/OSSettingStatusCell/helpers.ts` around lines 111 - 196, The status text still includes the "(pending)" suffix in several display configs; update the statusText values to remove that suffix for consistency. Specifically, in WINDOWS_DISK_ENCRYPTION_DISPLAY_CONFIG change the "pending" entry from "Enforcing (pending)" and "action_required" from "Action required (pending)" to remove "(pending)"; in LINUX_DISK_ENCRYPTION_DISPLAY_CONFIG change "action_required" similarly; and in RECOVERY_LOCK_PASSWORD_DISPLAY_CONFIG remove "(pending)" from the "pending" and "removing_enforcement" statusText values so all platforms render consistent statusText strings.
🧹 Nitpick comments (3)
frontend/pages/hosts/details/OSSettingsModal/OSSettingsTable/OSSettingsTableConfig.tsx (1)
94-97: Keep the column visually unlabeled, but add an accessible header label.Line 94 uses
Header: ""; this can degrade table navigation for screen readers. Consider rendering a visually hidden “Actions” header instead.Suggested patch
{ - Header: "", + Header: () => <span className="sr-only">Actions</span>, disableSortBy: true, accessor: "detail",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/pages/hosts/details/OSSettingsModal/OSSettingsTable/OSSettingsTableConfig.tsx` around lines 94 - 97, The column with accessor "detail" currently uses Header: "" which is invisible to screen readers; update the column definition in OSSettingsTableConfig (the column object with accessor "detail" and Cell) to provide an accessible header while keeping it visually hidden — e.g., replace the empty Header with a React header element that renders a visually hidden label like "Actions" (use your app's existing "sr-only" or visuallyHidden utility class) so sighted users see no label but assistive tech reads "Actions".frontend/pages/hosts/details/OSSettingsModal/OSSettingsTable/_styles.scss (1)
19-24: Add empty line before comment to satisfy linter.Stylelint reports a missing empty line before the comment on line 19.
🔧 Proposed fix
th.status__header { width: 200px; } + // Name column absorbs remaining space and truncates with ellipsis. tbody td.name__cell {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/pages/hosts/details/OSSettingsModal/OSSettingsTable/_styles.scss` around lines 19 - 24, Insert a blank line immediately before the comment "// Name column absorbs remaining space and truncates with ellipsis." in the SCSS block for tbody td.name__cell to satisfy Stylelint; locate the comment above the selector tbody td.name__cell and add one empty line between the previous line and that comment so the linter no longer reports a missing empty line before the comment.frontend/pages/hosts/details/OSSettingsModal/OSSettingsTable/OSSettingsResendCell/OSSettingsResendCell.tests.tsx (1)
12-111: Consider adding negative test cases for Resend button visibility.The test suite covers the happy paths for the Resend button but is missing negative test cases that mirror the Rotate button tests:
- Does not render a Resend button when
canResendProfilesisfalse- Does not render a Resend button when status is
pending- Does not render a Resend button for the recovery lock profile (
REC_LOCK_SYNTHETIC_PROFILE_UUID)These would ensure symmetry with the Rotate button tests and provide complete coverage for the visibility logic in
OSSettingsResendCell.tsx(lines 141-144).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/pages/hosts/details/OSSettingsModal/OSSettingsTable/OSSettingsResendCell/OSSettingsResendCell.tests.tsx` around lines 12 - 111, Add three negative test cases to OSSettingsResendCell.tests.tsx to mirror the Rotate tests: (1) render OSSettingsResendCell with canResendProfiles={false} and a non-recovery profile and assert screen.queryByRole("button", { name: "Resend" }) is not in the document; (2) render with canResendProfiles and profile status "pending" and assert the Resend button is not present; (3) render with canResendProfiles and profile_uuid set to REC_LOCK_SYNTHETIC_PROFILE_UUID and assert the Resend button is not present; use the existing createMockHostMdmProfile helper, noop for callbacks, and the same render/assert pattern used elsewhere in the file.
🤖 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/OSSettingsModal/OSSettingsTable/OSSettingsResendCell/OSSettingsResendCell.tsx`:
- Around line 18-21: The onClick prop in interface IResendButtonProps uses an
incorrect second generic for React.MouseEvent; update the type signature for
onClick (in IResendButtonProps) to use the native DOM event as the second
generic (e.g., React.MouseEvent<HTMLButtonElement, MouseEvent>) or omit the
second generic to use the default, ensuring the handler signature is (evt:
React.MouseEvent<HTMLButtonElement, MouseEvent>) => void.
- Around line 96-118: The onResendProfile handler is missing a success flash for
the non-Android path; after the await call to
resendRequest(profile.profile_uuid) in the else branch, call
renderFlash("success", "Successfully sent request to resend profile.") (or match
the Android message) and then call onProfileResent(), keeping the existing error
handling and the setIsResending toggles; update the else branch in
onResendProfile (which currently awaits resendRequest and calls onProfileResent)
to include the success renderFlash call.
---
Outside diff comments:
In
`@frontend/pages/hosts/details/OSSettingsModal/OSSettingsTable/OSSettingStatusCell/helpers.ts`:
- Around line 111-196: The status text still includes the "(pending)" suffix in
several display configs; update the statusText values to remove that suffix for
consistency. Specifically, in WINDOWS_DISK_ENCRYPTION_DISPLAY_CONFIG change the
"pending" entry from "Enforcing (pending)" and "action_required" from "Action
required (pending)" to remove "(pending)"; in
LINUX_DISK_ENCRYPTION_DISPLAY_CONFIG change "action_required" similarly; and in
RECOVERY_LOCK_PASSWORD_DISPLAY_CONFIG remove "(pending)" from the "pending" and
"removing_enforcement" statusText values so all platforms render consistent
statusText strings.
---
Nitpick comments:
In `@frontend/pages/hosts/details/OSSettingsModal/OSSettingsTable/_styles.scss`:
- Around line 19-24: Insert a blank line immediately before the comment "// Name
column absorbs remaining space and truncates with ellipsis." in the SCSS block
for tbody td.name__cell to satisfy Stylelint; locate the comment above the
selector tbody td.name__cell and add one empty line between the previous line
and that comment so the linter no longer reports a missing empty line before the
comment.
In
`@frontend/pages/hosts/details/OSSettingsModal/OSSettingsTable/OSSettingsResendCell/OSSettingsResendCell.tests.tsx`:
- Around line 12-111: Add three negative test cases to
OSSettingsResendCell.tests.tsx to mirror the Rotate tests: (1) render
OSSettingsResendCell with canResendProfiles={false} and a non-recovery profile
and assert screen.queryByRole("button", { name: "Resend" }) is not in the
document; (2) render with canResendProfiles and profile status "pending" and
assert the Resend button is not present; (3) render with canResendProfiles and
profile_uuid set to REC_LOCK_SYNTHETIC_PROFILE_UUID and assert the Resend button
is not present; use the existing createMockHostMdmProfile helper, noop for
callbacks, and the same render/assert pattern used elsewhere in the file.
In
`@frontend/pages/hosts/details/OSSettingsModal/OSSettingsTable/OSSettingsTableConfig.tsx`:
- Around line 94-97: The column with accessor "detail" currently uses Header: ""
which is invisible to screen readers; update the column definition in
OSSettingsTableConfig (the column object with accessor "detail" and Cell) to
provide an accessible header while keeping it visually hidden — e.g., replace
the empty Header with a React header element that renders a visually hidden
label like "Actions" (use your app's existing "sr-only" or visuallyHidden
utility class) so sighted users see no label but assistive tech reads "Actions".
🪄 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: 9bc5ab65-fffe-43e2-8cb0-1a0d4a1d6dc9
📒 Files selected for processing (15)
changes/40702-simplif-os-modalfrontend/pages/ManageControlsPage/OSSettings/cards/DiskEncryption/components/DiskEncryptionTable/DiskEncryptionTableConfig.tsxfrontend/pages/hosts/details/OSSettingsModal/OSSettingsTable/OSSettingStatusCell/OSSettingStatusCell.tests.tsxfrontend/pages/hosts/details/OSSettingsModal/OSSettingsTable/OSSettingStatusCell/OSSettingStatusCell.tsxfrontend/pages/hosts/details/OSSettingsModal/OSSettingsTable/OSSettingStatusCell/errorTooltipHelpers.tsxfrontend/pages/hosts/details/OSSettingsModal/OSSettingsTable/OSSettingStatusCell/helpers.tsfrontend/pages/hosts/details/OSSettingsModal/OSSettingsTable/OSSettingsErrorCell/OSSettingsErrorCell.tests.tsxfrontend/pages/hosts/details/OSSettingsModal/OSSettingsTable/OSSettingsErrorCell/index.tsfrontend/pages/hosts/details/OSSettingsModal/OSSettingsTable/OSSettingsNameCell/_styles.scssfrontend/pages/hosts/details/OSSettingsModal/OSSettingsTable/OSSettingsResendCell/OSSettingsResendCell.tests.tsxfrontend/pages/hosts/details/OSSettingsModal/OSSettingsTable/OSSettingsResendCell/OSSettingsResendCell.tsxfrontend/pages/hosts/details/OSSettingsModal/OSSettingsTable/OSSettingsResendCell/_styles.scssfrontend/pages/hosts/details/OSSettingsModal/OSSettingsTable/OSSettingsResendCell/index.tsfrontend/pages/hosts/details/OSSettingsModal/OSSettingsTable/OSSettingsTableConfig.tsxfrontend/pages/hosts/details/OSSettingsModal/OSSettingsTable/_styles.scss
💤 Files with no reviewable changes (2)
- frontend/pages/hosts/details/OSSettingsModal/OSSettingsTable/OSSettingsErrorCell/index.ts
- frontend/pages/hosts/details/OSSettingsModal/OSSettingsTable/OSSettingsErrorCell/OSSettingsErrorCell.tests.tsx
RachelElysia
approved these changes
Apr 9, 2026
RachelElysia
left a comment
RachelElysia
left a comment
Member
There was a problem hiding this comment.
I checked out the UI (without a failing OS) and looks great just like the code, tysm!
Member
Author
|
Thanks @RachelElysia 🤘 |
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.
Related issue: Resolves #40702
New look:
Checklist for submitter
If some of the following don't apply, delete the relevant line.
Changes file added for user-visible changes in
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Input data is properly validated,
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters.Timeouts are implemented and retries are limited to avoid infinite loops
If paths of existing endpoints are modified without backwards compatibility, checked the frontend/CLI for any necessary changes
Testing
Summary by CodeRabbit
Bug Fixes
New Features