iOS/iPadOS device vitals: "View all" vitals modal - #50147
Open
nulmete wants to merge 9 commits into
Open
Conversation
Adds a "View all" button to the Vitals card header for iOS/iPadOS hosts, opening a new modal that renders the 29 new device vitals alphabetically. The existing card is untouched — renderVitalsAlphabetically and every one of its per-field pushes are unchanged, so the card renders identically for every platform. Fields known unavailable for a host's enrollment method render a tooltip-wrapped "Not supported" treatment, driven by a static UNSUPPORTED_VITALS_BY_ENROLLMENT table keyed on mdm.enrollment_status rather than by inspecting rendered element props or any API signal. That table ships empty pending #49987's manual QA. The button and modal are wired through an optional toggleVitalsModal handler, matching the existing toggleLocationModal/toggleMDMStatusModal pattern, so the read-only My device page omits both.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 49985-ios-ipados-vitals-host-api #50147 +/- ##
====================================================================
+ Coverage 68.05% 68.09% +0.03%
====================================================================
Files 3930 3938 +8
Lines 250959 250845 -114
Branches 13361 13302 -59
====================================================================
+ Hits 170794 170803 +9
+ Misses 64833 64716 -117
+ Partials 15332 15326 -6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Narrow IosOrIpadosEnrollmentStatus out of MdmEnrollmentStatus with Extract instead of restating the three literals, so renaming or removing one of them in interfaces/mdm.ts fails to compile here rather than silently never matching a host. "On (company-owned)" stays out on purpose: it's the display name the UI shows for "On (automatic)" (MDM_ENROLLMENT_STATUS_UI_MAP), not a value the API returns — there's no such server constant and it isn't in the host_mdm.enrollment_status ENUM. Also trims comments and marks the pending-QA table and service subscriptions field with TODOs.
Member
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
WalkthroughAdds iOS/iPadOS Apple MDM vital types and optional host fields. Introduces shared vital construction and a modal that displays formatted vitals with enrollment-based unsupported handling. Adds an iOS/iPadOS-only “View all” button, host details modal state management, styling, and tests for rendering, formatting, unsupported values, and interaction. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@frontend/pages/hosts/details/modals/VitalsModal/unsupportedVitalsByEnrollment.ts`:
- Around line 53-68: Complete UNSUPPORTED_VITALS_BY_ENROLLMENT with the
QA-verified VitalKey values for each IosOrIpadosEnrollmentStatus instead of
leaving every mapping empty. Add fixed test fixtures covering supported and
unsupported vitals for every enrollment status, ensuring unsupported entries
render the “Not supported” treatment.
In `@frontend/pages/hosts/details/modals/VitalsModal/VitalsModal.tests.tsx`:
- Around line 212-222: Add a test alongside the existing “calls onExit when the
Done button is clicked” case in the VitalsModal suite that simulates pressing
Escape while the modal is open and asserts the onExit mock is called. Reuse
buildFullyPopulatedHost, createCustomRenderer, and the existing VitalsModal
setup, ensuring the assertion specifically covers keyboard dismissal.
🪄 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 Plus
Run ID: f5691a97-7f19-48a7-a7e8-3c6e12b0e8d9
📒 Files selected for processing (10)
frontend/interfaces/host.tsfrontend/pages/hosts/details/HostDetailsPage/HostDetailsPage.tsxfrontend/pages/hosts/details/cards/Vitals/Vitals.tests.tsxfrontend/pages/hosts/details/cards/Vitals/Vitals.tsxfrontend/pages/hosts/details/cards/Vitals/_styles.scssfrontend/pages/hosts/details/modals/VitalsModal/VitalsModal.tests.tsxfrontend/pages/hosts/details/modals/VitalsModal/VitalsModal.tsxfrontend/pages/hosts/details/modals/VitalsModal/_styles.scssfrontend/pages/hosts/details/modals/VitalsModal/index.tsfrontend/pages/hosts/details/modals/VitalsModal/unsupportedVitalsByEnrollment.ts
The suite asserted the Done button path only. Escape goes through Modal's handleClose instead, which defers onExit until the close animation finishes, so the assertion needs waitFor — a synchronous expect sees zero calls.
Product decisions from #39281: - The Vitals card now renders only the 8 vitals the Figma specifies for iOS/iPadOS. Every other platform still renders all of its vitals, with no "View all" button. "Enrollment ID" is allowlisted alongside "Serial number" because the two are mutually exclusive — without it a personally-enrolled (BYOD) host would be left with no identity row at all. - The modal lists the pre-existing vitals alongside the 29 iOS/iPadOS-only ones in one alphabetical list, so nothing the card trims becomes unreachable. renderVitalsAlphabetically is extracted to an exported buildHostVitals for both surfaces to share; most of the Vitals.tsx diff is the resulting reindentation rather than changed logic. - Nested-dict vitals (accessibility settings, organization info, MDM options) render as aligned "Label: value" lines instead of a JSON code block. Their schemas are closed, so each sub-key gets a hand-written label rather than a raw API key name. This also drops the last dangerouslySetInnerHTML here. - Service subscriptions render one underlined "Subscription N" per SIM, revealing that subscription's values on hover, matching the Hardware model tooltip treatment. Also fixes row dividers misaligning across the two columns when a value carries card-specific styling (.vitals-card__mdm-status sets a negative dd margin for its focus ring), and passes onEditCustomHostVital through so custom vitals stay editable from the modal.
nulmete
commented
Jul 29, 2026
| /** Builds the pre-existing host vitals as an unsorted list, so callers can | ||
| * filter (the card, for iOS/iPadOS) or extend (the "View all" modal, which | ||
| * appends the iOS/iPadOS-only vitals) before sorting. */ | ||
| export const buildHostVitals = ({ |
Member
Author
There was a problem hiding this comment.
this is basically the existing renderVitalsAlphabetically but now exported so it can be re-used in the VitalsModal. Kept here to not make the diff even bigger.
Copy specified in #39281, for Awaiting configuration, EAS device identifier, Push token, Supplemental build version, and Supplemental OS version extra. Labels stay plain strings for sorting; the tooltip wrapper is presentation only. Enrollment ID needed no change — its tooltip already lives in buildHostVitals, so sharing that builder means the card and the modal both render it. Covered by a test, since that seam is what the extraction introduced.
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
frontend/pages/hosts/details/modals/VitalsModal/VitalsModal.tsx (2)
99-115: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueKey
renderLinesitems by index.Using the line content as the key means a repeated value in a list vital (e.g. two identical attestation entries) yields duplicate React keys and a dropped/warned row. Index keys are safe here since the list is static and never reordered.
♻️ Proposed change
- {lines.map((line) => ( - <div key={line}>{line}</div> + {lines.map((line, i) => ( + // eslint-disable-next-line react/no-array-index-key + <div key={`${i}-${line}`}>{line}</div> ))}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/pages/hosts/details/modals/VitalsModal/VitalsModal.tsx` around lines 99 - 115, Update renderLines so each mapped line uses its array index as the React key instead of the line content, preventing duplicate keys when repeated vital values occur. Keep the existing filtering, empty-value fallback, and rendered line content unchanged.
412-412: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant import-time mutation.
sortHostVitalsalready orders the merged list by the samelabel/localeCompare, so this in-place sort of a module-levelconsthas no effect on output. Dropping it removes the import-time side effect.♻️ Proposed change
-VITALS.sort((a, b) => a.label.localeCompare(b.label));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/pages/hosts/details/modals/VitalsModal/VitalsModal.tsx` at line 412, Remove the module-level VITALS.sort call. The existing sortHostVitals function already orders the merged vitals by label with localeCompare, so preserve that ordering while eliminating the redundant import-time mutation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/pages/hosts/details/modals/VitalsModal/VitalsModal.tsx`:
- Around line 167-188: Update the subscription list rendered by
renderServiceSubscriptions to use the map index as each TooltipWrapper key
instead of sub.slot, ensuring keys remain defined and unique for unprovisioned
or duplicate-slot subscriptions.
---
Nitpick comments:
In `@frontend/pages/hosts/details/modals/VitalsModal/VitalsModal.tsx`:
- Around line 99-115: Update renderLines so each mapped line uses its array
index as the React key instead of the line content, preventing duplicate keys
when repeated vital values occur. Keep the existing filtering, empty-value
fallback, and rendered line content unchanged.
- Line 412: Remove the module-level VITALS.sort call. The existing
sortHostVitals function already orders the merged vitals by label with
localeCompare, so preserve that ordering while eliminating the redundant
import-time mutation.
🪄 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 Plus
Run ID: c85929ef-ae11-4e2d-8674-1d9ecd21096c
📒 Files selected for processing (6)
frontend/pages/hosts/details/HostDetailsPage/HostDetailsPage.tsxfrontend/pages/hosts/details/cards/Vitals/Vitals.tests.tsxfrontend/pages/hosts/details/cards/Vitals/Vitals.tsxfrontend/pages/hosts/details/modals/VitalsModal/VitalsModal.tests.tsxfrontend/pages/hosts/details/modals/VitalsModal/VitalsModal.tsxfrontend/pages/hosts/details/modals/VitalsModal/_styles.scss
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/pages/hosts/details/HostDetailsPage/HostDetailsPage.tsx
- frontend/pages/hosts/details/modals/VitalsModal/_styles.scss
Comment on lines
+167
to
+188
| const renderServiceSubscriptions = ( | ||
| subscriptions?: IHostMdmAppleServiceSubscription[] | ||
| ) => { | ||
| if (!subscriptions?.length) { | ||
| return EMPTY_VITAL_VALUE; | ||
| } | ||
|
|
||
| return ( | ||
| <div className={`${baseClass}__subscriptions`}> | ||
| {subscriptions.map((sub, i) => ( | ||
| <TooltipWrapper | ||
| key={sub.slot} | ||
| tipContent={renderTooltipFields(subscriptionFields(sub))} | ||
| > | ||
| {/* Template literal keeps this one text node, so the label stays | ||
| queryable as a whole string. */} | ||
| {`Subscription ${i + 1}`} | ||
| </TooltipWrapper> | ||
| ))} | ||
| </div> | ||
| ); | ||
| }; |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
key={sub.slot} can be undefined or duplicated.
slot is optional (the file's own comments note subscriptions with no slot), so an unprovisioned entry yields key={undefined}, and two such entries collide. Key by index instead — the list is static and never reordered.
🐛 Proposed fix
{subscriptions.map((sub, i) => (
<TooltipWrapper
- key={sub.slot}
+ // eslint-disable-next-line react/no-array-index-key
+ key={sub.slot ?? `subscription-${i}`}
tipContent={renderTooltipFields(subscriptionFields(sub))}
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const renderServiceSubscriptions = ( | |
| subscriptions?: IHostMdmAppleServiceSubscription[] | |
| ) => { | |
| if (!subscriptions?.length) { | |
| return EMPTY_VITAL_VALUE; | |
| } | |
| return ( | |
| <div className={`${baseClass}__subscriptions`}> | |
| {subscriptions.map((sub, i) => ( | |
| <TooltipWrapper | |
| key={sub.slot} | |
| tipContent={renderTooltipFields(subscriptionFields(sub))} | |
| > | |
| {/* Template literal keeps this one text node, so the label stays | |
| queryable as a whole string. */} | |
| {`Subscription ${i + 1}`} | |
| </TooltipWrapper> | |
| ))} | |
| </div> | |
| ); | |
| }; | |
| const renderServiceSubscriptions = ( | |
| subscriptions?: IHostMdmAppleServiceSubscription[] | |
| ) => { | |
| if (!subscriptions?.length) { | |
| return EMPTY_VITAL_VALUE; | |
| } | |
| return ( | |
| <div className={`${baseClass}__subscriptions`}> | |
| {subscriptions.map((sub, i) => ( | |
| <TooltipWrapper | |
| // eslint-disable-next-line react/no-array-index-key | |
| key={sub.slot ?? `subscription-${i}`} | |
| tipContent={renderTooltipFields(subscriptionFields(sub))} | |
| > | |
| {/* Template literal keeps this one text node, so the label stays | |
| queryable as a whole string. */} | |
| {`Subscription ${i + 1}`} | |
| </TooltipWrapper> | |
| ))} | |
| </div> | |
| ); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/pages/hosts/details/modals/VitalsModal/VitalsModal.tsx` around lines
167 - 188, Update the subscription list rendered by renderServiceSubscriptions
to use the map index as each TooltipWrapper key instead of sub.slot, ensuring
keys remain defined and unique for unprovisioned or duplicate-slot
subscriptions.
Member
Author
There was a problem hiding this comment.
per my testing, slot is always returned from the command.
It described behaviour from when renderLines rendered service subscriptions by slot; it now only renders the attestation certificate chain. The stale mention of "a subscription with no slot" implied slot is optional, which it isn't — it's non-null and part of the (host_uuid, slot) primary key.
…' into 49986-ios-ipados-vitals-view-all # Conflicts: # frontend/pages/hosts/details/cards/Vitals/Vitals.tsx
Testing across three devices found accessibility_settings absent on every non-ADE enrollment (BYOD, manual, and a fresh manual re-enrollment) and present on both company-owned ones. But supervision and ADE never varied independently in that sample — every ADE device is supervised — so the difference can't be attributed to the enrollment method, which is what the table is keyed on and what its tooltip asserts. A manually-enrolled device supervised via Apple Configurator would make that message wrong. Rendering the empty-value text instead is accurate either way, so remove the mechanism and let QA's per-enrollment sweep decide whether it's needed. Restore from c69f748 (the mechanism) and 6896570 (the Extract-derived enrollment status type) if it is. VitalKey moves into VitalsModal.tsx, its only consumer, as it was the last export of unsupportedVitalsByEnrollment.ts still in use.
Treat the 8 card vitals as a priority list rather than a hard filter, so a host reporting only some of them fills the remaining slots with its other pre-existing vitals instead of rendering a short card. The 29 iOS/iPadOS-only vitals are built by the "View all" modal rather than buildHostVitals, so they can never backfill into the card. Adds the timezone field to IHost, which the API has always returned but the interface was missing.
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
1 participant
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 #49986
Adds a "View all" button to the Vitals card for iOS/iPadOS hosts, opening a new modal that lists the 29 new device vitals alphabetically.
Fields unavailable for a host's enrollment method show a tooltipped "Not supported" treatment.I didn't notice any field being not supported based on different tests that were run on this branch (my own iPhone, and Magnus/Rachel's iPad/iPhones). If during QA we detect some fields not being supported for certain enrollments, I can definitely add that code back.Checklist for submitter
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Will be added in the feature branch.
Testing
Added/updated automated tests
QA'd all new/changed functionality manually
Summary by CodeRabbit
New Features
Bug Fixes