Open
Conversation
Member
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #42608 +/- ##
=======================================
Coverage 66.67% 66.67%
=======================================
Files 2533 2535 +2
Lines 203216 203249 +33
Branches 9231 9229 -2
=======================================
+ Hits 135485 135515 +30
- Misses 55469 55473 +4
+ Partials 12262 12261 -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:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new host activity event for Android certificate installation outcomes, and exposes host activity UI for Android devices so admins can see those events on the host details page.
Changes:
- Logs a new
installed_certificateactivity when an Android host reports a terminal certificate status (verified/failed). - Adds backend activity type plumbing + frontend rendering for the new activity item.
- Enables the host Activity card on Android host details.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/certificates.go | Logs an installed_certificate activity after persisting certificate status updates. |
| server/fleet/certificate_templates.go | Introduces CertificateActivityStatus constants used by the service when logging activities. |
| server/fleet/activities.go | Registers ActivityTypeInstalledCertificate for activity serialization/handling. |
| frontend/pages/hosts/details/cards/Activity/ActivityItems/InstalledCertificateActivityItem/index.ts | Re-exports the new activity item component. |
| frontend/pages/hosts/details/cards/Activity/ActivityItems/InstalledCertificateActivityItem/InstalledCertificateActivityItem.tsx | Renders the “installed/failed to install certificate” host activity UI (including optional detail). |
| frontend/pages/hosts/details/cards/Activity/ActivityConfig.tsx | Wires the new activity type to the host “past activities” component map. |
| frontend/pages/hosts/details/HostDetailsPage/HostDetailsPage.tsx | Removes Android gating so the Activity card renders for Android hosts. |
| frontend/interfaces/activity.ts | Adds InstalledCertificate to ActivityType, host past activity union, and filter label; adds detail to activity details typing. |
| changes/37546-android-certificate-install-activity | Changelog entry for activity logging + enabling Activity card on Android. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
WalkthroughThis pull request implements activity logging for certificate installation on Android devices. It introduces a new 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 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: 4
🤖 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/Activity/ActivityItems/InstalledCertificateActivityItem/InstalledCertificateActivityItem.tsx`:
- Around line 25-27: Replace the shorthand fragment used to render the detail
string in the InstalledCertificateActivityItem component with a real element to
satisfy Prettier: change the JSX block that checks activity.details?.detail and
currently returns <> Detail: {activity.details.detail}</> to use a <span> (or
React.Fragment with explicit tag) instead, e.g. {activity.details?.detail &&
(<span>Detail: {activity.details.detail}</span>)} so the fragment shorthand is
removed and formatting/lint will pass.
- Around line 12-34: The current ternary uses isFailed only and treats any
other/missing status as a success; update the status handling in
InstalledCertificateActivityItem by explicitly checking for known states:
compute const isFailed = activity.details?.status === "failed_install" and const
isInstalled = activity.details?.status === "installed" (or similar explicit
checks), then render three branches: failed (if isFailed), success (only if
isInstalled), and a safe fallback/neutral message when activity.details?.status
is absent or unknown (e.g., "certificate installation status unknown" or omit
the success claim); adjust references to activity.details?.certificate_name and
activity.details?.detail accordingly.
In `@server/service/certificates.go`:
- Around line 698-700: The status update path currently calls
svc.ds.UpsertCertificateStatus(...) then calls NewActivity and returns an error
if activity creation fails, which causes the endpoint to fail even though the
status is persisted; change this so that after svc.ds.UpsertCertificateStatus
succeeds you do not return an error when NewActivity (or
activityStore.NewActivity) fails—instead treat activity creation as best-effort:
catch/log the NewActivity error (include context like certificate ID and new
status) and continue returning success, or alternatively wrap
UpsertCertificateStatus and NewActivity in a single transactional/atomic
operation if your storage supports it so both succeed or both roll back; update
all similar code paths (including the block referenced at lines 715-724) to
follow the same pattern using the svc.ds.UpsertCertificateStatus and NewActivity
symbols.
- Around line 702-709: The mapping of MDMDeliveryVerified/MDMDeliveryFailed to
CertificateActivityInstalled/CertificateActivityFailedInstall in
UpdateCertificateStatus must only apply to install operations; guard this logic
by checking the certificate operation type (use the update.OperationType field)
and only execute the switch when update.OperationType indicates an install
(e.g., fleet.CertificateOperationInstall); if the operation is a removal, skip
this install-specific mapping so failed removals are not reported as failed
installs.
🪄 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: b622c9e8-6c9a-49a8-a1f6-53791297b35c
📒 Files selected for processing (9)
changes/37546-android-certificate-install-activityfrontend/interfaces/activity.tsfrontend/pages/hosts/details/HostDetailsPage/HostDetailsPage.tsxfrontend/pages/hosts/details/cards/Activity/ActivityConfig.tsxfrontend/pages/hosts/details/cards/Activity/ActivityItems/InstalledCertificateActivityItem/InstalledCertificateActivityItem.tsxfrontend/pages/hosts/details/cards/Activity/ActivityItems/InstalledCertificateActivityItem/index.tsserver/fleet/activities.goserver/fleet/certificate_templates.goserver/service/certificates.go
...Activity/ActivityItems/InstalledCertificateActivityItem/InstalledCertificateActivityItem.tsx
Show resolved
Hide resolved
...Activity/ActivityItems/InstalledCertificateActivityItem/InstalledCertificateActivityItem.tsx
Outdated
Show resolved
Hide resolved
![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.
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 #37546
Docs: #42609
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.Testing
Summary by CodeRabbit