Show configuration profile name and more fine-grained status - #42126
Hidden character warning
Conversation
Zed + Opus 4.6; prompt: Implement #40177. Check subtasks in GitHub for further implementation details, and pull from Figma (linked in the main ticket) to see what visuals should look like.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #42126 +/- ##
=======================================
Coverage 66.86% 66.87%
=======================================
Files 2587 2588 +1
Lines 207476 207563 +87
Branches 9163 9206 +43
=======================================
+ Hits 138737 138803 +66
- Misses 56110 56124 +14
- Partials 12629 12636 +7
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[agent] Can you fix the CI issues here? Commit directly to the branch with any fixes. |
# Conflicts: # server/datastore/mysql/schema.sql
…lding a brand new one
When pulling name data we refer to the underlying tables so we don't need this in the view
Slight perf optimization to avoid thundering herds
This reverts commit 5ee3115. Turns out, we need this.
| if enqueued && target.ProfileName != "" { | ||
| if err := ds.SetCommandName(ctx, target.CmdUUID, target.ProfileName); err != nil { | ||
| logger.ErrorContext(ctx, "setting command name for profile", "command_uuid", target.CmdUUID, "profile_name", target.ProfileName, "err", err) | ||
| } |
There was a problem hiding this comment.
@JordanMontgomery Is this (plus the associated UPDATE) acceptable perf-wise or do I need to did into nanomdm's call stack and pass the profile name all the way through?
There was a problem hiding this comment.
I would honestly pass it all the way through to avoid duplicating writes. This can be pretty intensive in certain cases
There was a problem hiding this comment.
Got it, thanks. I'll dig in on that refactor. /cc @AndreyKizimenko that we'll absolutely want MDM smoke and macOS load tests for 4.84 as a result of these changes; I don't expect a regression given our test coverage but I'll be editing load-bearing code as a part of this.
| logger *slog.Logger, | ||
| assocs []fleet.SCEPIdentityAssociation, | ||
| profile []byte, | ||
| profileName string, |
There was a problem hiding this comment.
@JordanMontgomery Is this what we want here, or something different?
# Conflicts: # server/datastore/mysql/schema.sql
# Conflicts: # server/datastore/mysql/schema.sql
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| } | ||
| if migrationEnrollmentProfile != "" && hasAssocsFromMigration { | ||
| profileBytes := []byte(migrationEnrollmentProfile) | ||
| if err := renewMDMAppleEnrollmentProfile(ctx, ds, commander, logger, assocsFromMigration, profileBytes); err != nil { | ||
| if err := renewMDMAppleEnrollmentProfile(ctx, ds, commander, logger, assocsFromMigration, profileBytes, appConfig.OrgInfo.OrgName+" migration enrollment"); err != nil { |
There was a problem hiding this comment.
🟡 The PR adds cmd.Name assertions for 4 of the 5 renewMDMAppleEnrollmentProfile call sites in RenewSCEPCertificates, but the 5th call site (line 5828, which passes OrgName + " migration enrollment") is never exercised by any test because the code path is gated on FLEET_SILENT_MIGRATION_ENROLLMENT_PROFILE being non-empty, and no test sets that env var. A typo or regression in the " migration enrollment" suffix string would go undetected. Fix by adding a test case in TestRenewSCEPCertificatesBranches that sets the env var with t.Setenv() and asserts cmd.Name equals the expected value.
Extended reasoning...
What the gap is and how it manifests
The PR deliberately introduces cmd.Name assertions across the call sites to renewMDMAppleEnrollmentProfile in RenewSCEPCertificates. Reviewing the diff against apple_mdm_test.go, assertions exist for:
appConfig.OrgInfo.OrgName + " enrollment"(hostsWithoutRefs, hostsWithRefs — tested twice each)appConfig.OrgInfo.OrgName + " account driven enrollment"(userDeviceAssocs — tested twice)appConfig.OrgInfo.OrgName + " ACME enrollment"(tested inTestRenewACMECertificatesBranches)
The 5th call site in apple_mdm.go at lines 5825–5828 — which passes appConfig.OrgInfo.OrgName + " migration enrollment" — is not covered by any test.
The specific code path that is untested
The migration enrollment branch is guarded by:
if migrationEnrollmentProfile != "" && hasAssocsFromMigration {
...
if err := renewMDMAppleEnrollmentProfile(..., appConfig.OrgInfo.OrgName+" migration enrollment"); err != nil {migrationEnrollmentProfile is populated from os.Getenv("FLEET_SILENT_MIGRATION_ENROLLMENT_PROFILE"). No test case in TestRenewSCEPCertificatesBranches or TestRenewACMECertificatesBranches calls t.Setenv("FLEET_SILENT_MIGRATION_ENROLLMENT_PROFILE", ...), so this branch is permanently skipped in all CI runs. A grep for the env var name in the test file confirms zero matches.
Why existing code does not detect this
Because the path requires a non-empty env var plus hasAssocsFromMigration being true (which requires specific datastore mock setup), it cannot be triggered without an explicit test case. The four other call sites are reachable through the existing test fixture paths, so they have coverage; this one does not. The PR introduced assertions for all the other cases and therefore created a visible asymmetry.
Addressing the refutation
One verifier argued this is "not a real bug" because the production string literal is plainly correct and visible. That is a fair point — the production code is functioning correctly as written. This is a test coverage gap, not a functional defect. However, the PR's explicit intent is to verify that cmd.Name is correct at each call site, and failing to cover one of five call sites is an inconsistency in that stated intent. A future refactor that accidentally corrupts the suffix for this branch would not be caught by the test suite.
How to fix it
Add a test branch in TestRenewSCEPCertificatesBranches that:
- Calls
t.Setenv("FLEET_SILENT_MIGRATION_ENROLLMENT_PROFILE", someValidProfilePlist) - Configures the datastore mock to return associations tagged as migration associations (
hasAssocsFromMigration = true) - Asserts
cmd.Name == appConfig.OrgInfo.OrgName + " migration enrollment"in theEnqueueCommandFuncmock
Step-by-step demonstration of the gap
- PR reviewer reads the test file and sees
cmd.Nameassertions for" enrollment"," account driven enrollment", and" ACME enrollment". - Reviewer searches for
" migration enrollment"in the test file — no matches. - Developer renames the suffix to
" migration_enrollment"(adds underscore) inapple_mdm.goline 5828. - All existing tests still pass, because the migration branch never runs.
- At runtime, the wrong command name is stored in
nano_commands.namefor migration enrollment hosts, causing incorrect display in the Fleet UI for that subset of devices.
… to max-height Zed + Opus 4.6; prompts: > On the MDM command details modal, the default height for the text area input is used even when the text being displayed is much longer. What are we missing style-wise to get the text area to expand to fill content, subject to max-height? ... < Since these fields in the `CommandDetailsModal` are read-only with `enableCopy`, option 1 (switching to the `Textarea` component) would be the most pragmatic fix — it already handles this correctly and is used in a similar context in `SoftwareIpaInstallDetailsModal`. Would you like me to make either change? > Yes (it picked the smaller change)
| const isProfileCommand = (requestType: string): boolean => | ||
| requestType === "InstallProfile" || requestType === "RemoveProfile"; |
There was a problem hiding this comment.
🟡 The isProfileCommand(requestType) helper defined at lines 21–22 of CommandItem.tsx is never called anywhere in the file and should be removed. The actual rendering logic uses a direct name ? ... : ... ternary for profile name display and getStatusText() reads command_status/status directly, making this function dead code that implies profile-specific guard logic exists when it does not.
Extended reasoning...
What the dead code is
The function isProfileCommand is defined at lines 21–22 of CommandItem.tsx:
const isProfileCommand = (requestType: string): boolean =>
requestType === "InstallProfile" || requestType === "RemoveProfile";It is never invoked anywhere in the file or the broader frontend codebase (confirmed by grep).
The specific code paths that ignore it
The PR implements two pieces of logic that might plausibly use isProfileCommand, but neither does:
- Profile name display – The
activityTextJSX uses a plainname ? (...) : (...)ternary (lines 51–59 of the modified file). It does not gate onisProfileCommand(request_type). - Status text –
getStatusText()inspects only thecommand_statusandstatusfields fromICommand. It makes no distinction between profile and non-profile command types.
Why existing code doesn't detect this
TypeScript does not warn about unused function declarations at module scope. The helper is exported-free, so no import check catches it either. It compiles and ships silently.
What the impact would be
No functional impact — the function is genuinely unreachable. The issue is purely readability/maintainability: the presence of isProfileCommand implies that profile-specific branching was intended somewhere in the status or display logic, leaving future maintainers uncertain whether the omission of a call was intentional or a latent bug.
How to fix it
Delete the two-line declaration entirely:
-const isProfileCommand = (requestType: string): boolean =>
- requestType === "InstallProfile" || requestType === "RemoveProfile";Step-by-step proof
- Search for all usages of
isProfileCommandinCommandItem.tsx— only the definition at lines 21–22 appears; no call sites exist. - Grep the broader
frontend/directory forisProfileCommand— zero results outside this file. - Review
getStatusText(): it switches oncommand_statusand checksstatus === "NotNow"; no call toisProfileCommand. - Review
activityText: it usesname ?directly; no call toisProfileCommand. - Conclusion: the function is dead code introduced (or left over from an earlier draft) by this PR.
There was a problem hiding this comment.
I'll fix this in a follow-up PR
Resolves #40177 and subissues. If some of the following don't apply, delete the relevant line. - [x] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. See [Changes files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/guides/committing-changes.md#changes-files) for more information. - [x] 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. - [x] Added/updated automated tests - [x] Where appropriate, [automated tests simulate multiple hosts and test for host isolation](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/reference/patterns-backend.md#unit-testing) (updates to one hosts's records do not affect another) - [sorta] QA'd all new/changed functionality manually - [x] Checked schema for all modified table for columns that will auto-update timestamps during migration. - [x] Confirmed that updating the timestamps is acceptable, and will not cause unwanted side effects. - [x] Ensured the correct collation is explicitly set for character columns (`COLLATE utf8mb4_unicode_ci`). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> * **New Features** * Profile names are now displayed alongside mobile device management commands for installing or removing profiles. These names are visible in command details modals and within device activity timelines. * Added "NotNow" status for deferred profile commands, providing improved transparency into which profiles are being managed and the current status of profile installation or removal operations. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
…atus (#43348) Merged into `main` in #42126 and #43345. Conflict resolution was required due to DB migration + recovery lock features that are landing in 4.85 that this work touched, but that resolution was basically "don't include this function and don't include the corersponding test" so not a big deal.
Resolves #40177 and subissues.
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.Testing
Added/updated automated tests
Where appropriate, automated tests simulate multiple hosts and test for host isolation (updates to one hosts's records do not affect another)
[sorta] QA'd all new/changed functionality manually
Database migrations
COLLATE utf8mb4_unicode_ci).Summary by CodeRabbit