Add activities for user actions on labels#44522
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 #44522 +/- ##
==========================================
+ Coverage 66.76% 66.80% +0.04%
==========================================
Files 2636 2637 +1
Lines 211834 212402 +568
Branches 9425 9568 +143
==========================================
+ Hits 141437 141902 +465
- Misses 57551 57604 +53
- Partials 12846 12896 +50
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:
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds activity generation for label lifecycle events by introducing three new activity detail types (created_label, edited_label, deleted_label). Backend: new datastore method to fetch label membership host IDs, datastore interface and mock extensions, service logic to emit activities on label create/modify/delete, host label add/remove, and ApplyLabelSpecs comparisons to detect semantic changes before emitting activities. Frontend: new activity types, extended activity detail fields, filter mappings, UI templates, and tests to render label activities. Tests and GitOps integration updated to capture and validate emitted label activities. 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/service/hosts.go`:
- Around line 3629-3673: emitEditedLabelActivities can emit duplicate activities
when labelNames contains duplicates; before the final loop over labelNames
deduplicate them (e.g., build a seen map[string]bool and either produce an
ordered slice of unique names or skip repeats during iteration) so you only call
svc.NewActivity once per distinct label name; use the existing labels map lookup
(labels[name]) and keep the rest of the logic (fleetName resolution,
svc.NewActivity) unchanged.
In `@server/service/labels.go`:
- Around line 816-827: The function labelSpecMatchesLabel currently ignores
platform-only changes so add a platform comparison to detect those edits: in
labelSpecMatchesLabel (params *fleet.LabelSpec, *fleet.Label) compare
spec.Platforms (or spec.Platform) with label.Platforms (or label.Platform) and
return false when they differ; for slice fields use a slice-equality check
(e.g., reflect.DeepEqual) and place this check before the HostVitalsCriteria
comparison so platform-only changes cause the function to return false and
trigger edited_label activity.
🪄 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: 9532040c-16de-4191-9777-6514d20c3a30
📒 Files selected for processing (15)
changes/36976-activities-for-labelscmd/fleetctl/fleetctl/apply_test.gocmd/fleetctl/fleetctl/testing_utils/testing_utils.gocmd/fleetctl/integrationtest/gitops/gitops_enterprise_integration_test.gofrontend/interfaces/activity.tsfrontend/pages/DashboardPage/cards/ActivityFeed/GlobalActivityItem/GlobalActivityItem.tests.tsxfrontend/pages/DashboardPage/cards/ActivityFeed/GlobalActivityItem/GlobalActivityItem.tsxserver/datastore/mysql/labels.goserver/datastore/mysql/labels_test.goserver/fleet/activities.goserver/fleet/datastore.goserver/mock/datastore_mock.goserver/service/hosts.goserver/service/labels.goserver/service/labels_test.go
There was a problem hiding this comment.
Pull request overview
This PR adds first-class activity feed events for label lifecycle changes (create/edit/delete) across UI actions, GitOps spec apply, and host manual-label membership changes, and renders those new activities in the Dashboard Activity Feed.
Changes:
- Add new activity types:
created_label,edited_label,deleted_label, and emit them from relevant service paths (label CRUD, ApplyLabelSpecs, add/remove labels on host). - Add unfiltered label membership host-ID lookup to support “manual label hosts changed” detection during GitOps apply.
- Update frontend activity feed templates/types/tests to display and filter the new label activities.
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/labels.go | Emits label activities on create/edit/delete and GitOps spec apply, including change detection helpers. |
| server/service/hosts.go | Emits edited_label activities when manual labels are added/removed from a host. |
| server/fleet/activities.go | Registers new activity detail types for label actions. |
| server/fleet/datastore.go | Extends datastore interface with LabelMembershipHostIDs. |
| server/mock/datastore_mock.go | Adds mock support for LabelMembershipHostIDs. |
| server/datastore/mysql/labels.go | Implements LabelMembershipHostIDs in MySQL datastore. |
| server/datastore/mysql/labels_test.go | Adds datastore test coverage for unfiltered membership lookup. |
| server/service/labels_test.go | Adds/updates unit tests verifying label activity emission paths. |
| frontend/pages/DashboardPage/cards/ActivityFeed/GlobalActivityItem/GlobalActivityItem.tsx | Renders new label activities in the global activity feed. |
| frontend/pages/DashboardPage/cards/ActivityFeed/GlobalActivityItem/GlobalActivityItem.tests.tsx | Adds frontend test coverage for new activity renderings. |
| frontend/interfaces/activity.ts | Adds new activity enum values, details fields, and filter-label mappings. |
| cmd/fleetctl/integrationtest/gitops/gitops_enterprise_integration_test.go | Adds GitOps integration test that validates per-label activities across create/edit/no-op/delete scenarios. |
| cmd/fleetctl/fleetctl/testing_utils/testing_utils.go | Extends default mocked DS behavior to support new ApplyLabelSpecs activity detection paths. |
| cmd/fleetctl/fleetctl/apply_test.go | Adjusts invocation tracking for LabelsByName to account for new calls. |
| changes/36976-activities-for-labels | Adds user-visible change entry for the new label activities. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
server/service/labels.go (1)
816-827:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCompare
platformin spec-vs-label matching to avoid missing real edit activities.Platform-only GitOps updates currently evaluate as unchanged, so
edited_labelis not emitted for a real label edit.Suggested fix
func labelSpecMatchesLabel(spec *fleet.LabelSpec, label *fleet.Label) bool { if spec.Description != label.Description { return false } + if spec.Platform != label.Platform { + return false + } if spec.Query != label.Query { return false } if spec.LabelMembershipType != label.LabelMembershipType { return false } return jsonRawMessageEqual(spec.HostVitalsCriteria, label.HostVitalsCriteria) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/labels.go` around lines 816 - 827, The labelSpecMatchesLabel function currently omits comparing the platform fields, causing platform-only GitOps updates to be treated as unchanged; update labelSpecMatchesLabel (used to detect edits) to also compare spec.Platform with label.Platform (e.g., spec.Platform != label.Platform) as part of the equality checks so that a difference in platform triggers a non-match and allows edited_label to be emitted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@server/service/labels.go`:
- Around line 816-827: The labelSpecMatchesLabel function currently omits
comparing the platform fields, causing platform-only GitOps updates to be
treated as unchanged; update labelSpecMatchesLabel (used to detect edits) to
also compare spec.Platform with label.Platform (e.g., spec.Platform !=
label.Platform) as part of the equality checks so that a difference in platform
triggers a non-match and allows edited_label to be emitted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 48c97a94-7090-47de-94d4-825eaff8a13f
📒 Files selected for processing (2)
cmd/fleetctl/integrationtest/gitops/gitops_enterprise_integration_test.goserver/service/labels.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server/service/labels.go (1)
195-230:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSkip
edited_labelfor no-opModifyLabelrequests.This path now emits an edit activity even when the payload doesn't change the label at all (empty payload, same name/description, same manual host set). That will create false audit entries and is inconsistent with the semantic no-op check added in
ApplyLabelSpecs. Compare against the pre-update state and return withoutSaveLabel/NewActivitywhen nothing changed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/labels.go` around lines 195 - 230, The ModifyLabel flow currently always calls SaveLabel and NewActivity even when the incoming payload makes no changes; update the logic in the ModifyLabel handler to first compute whether any actual changes would occur (compare the incoming payload fields — name, description, label.LabelMembershipType, and the resolved hostIDs — against the existing label state and its current membership set obtained from the DS), and if nothing would change, return early without calling svc.ds.UpdateLabelMembershipByHostIDs, svc.ds.SaveLabel, or svc.NewActivity; reuse the existing variables (label, payload, hostIDs, savedHostIDs) and the same filter/context so the check mirrors ApplyLabelSpecs behavior and only proceeds to UpdateLabelMembershipByHostIDs/SaveLabel/NewActivity when a real change is detected.server/service/hosts.go (1)
3581-3585:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOnly emit
edited_labelwhen the host's membership actually changed.Both paths now log an edit for every requested label name, even when the datastore write is a no-op (for example, adding a label the host already has or removing one it never had). That makes the activity feed inaccurate and easy to spam. Capture the host's label set before the write and emit activities only for labels whose membership changed.
Also applies to: 3621-3625
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/hosts.go` around lines 3581 - 3585, Before calling svc.ds.AddLabelsToHost, read the host's current label membership (e.g., via host.LabelIDs or a datastore reader like svc.ds.GetLabelsForHost) and compute which of the requested labelIDs/labelNames will actually change membership; after the AddLabelsToHost call only invoke svc.emitEditedLabelActivities with the subset of labelNames that were added or removed (i.e., whose membership toggled). Apply the same change to the other occurrence around svc.ds.RemoveLabelsFromHost / the block referenced at 3621-3625 so emitEditedLabelActivities is only called for labels with actual membership changes.
🧹 Nitpick comments (1)
cmd/fleetctl/integrationtest/gitops/gitops_enterprise_integration_test.go (1)
3382-3539: ⚡ Quick winCover the remaining team-scoped label types in this GitOps activity test.
This test only exercises team label activities with a dynamic label. The PR scope also includes team-scoped manual and host_vitals/IdP-created labels, so a regression in either path would still pass here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/fleetctl/integrationtest/gitops/gitops_enterprise_integration_test.go` around lines 3382 - 3539, The test currently only exercises team-scoped dynamic labels; extend it to cover team-scoped manual and host_vitals/IdP-created label flows by adding parallel phases that call writeTeam (in addition to writeGlobal) to create a team manual label (with hosts list) and a team host_vitals label (with criteria), then run apply() and flush() and assert the emitted activities include ActivityTypeCreatedLabel, ActivityTypeEditedLabel and ActivityTypeDeletedLabel for those team labels (verifying FleetID/FleetName presence for team-scoped activities), and also include no-op re-apply checks and edits (change query -> change hosts -> change criteria) using the same pattern as existing global label phases; locate changes around the existing writeTeam/writeGlobal/apply/flush sequences and reuse the same assertions that check types (ActivityTypeCreatedLabel, ActivityTypeEditedLabel, ActivityTypeDeletedLabel) and name membership to ensure team manual and host_vitals paths are covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@server/service/hosts.go`:
- Around line 3581-3585: Before calling svc.ds.AddLabelsToHost, read the host's
current label membership (e.g., via host.LabelIDs or a datastore reader like
svc.ds.GetLabelsForHost) and compute which of the requested labelIDs/labelNames
will actually change membership; after the AddLabelsToHost call only invoke
svc.emitEditedLabelActivities with the subset of labelNames that were added or
removed (i.e., whose membership toggled). Apply the same change to the other
occurrence around svc.ds.RemoveLabelsFromHost / the block referenced at
3621-3625 so emitEditedLabelActivities is only called for labels with actual
membership changes.
In `@server/service/labels.go`:
- Around line 195-230: The ModifyLabel flow currently always calls SaveLabel and
NewActivity even when the incoming payload makes no changes; update the logic in
the ModifyLabel handler to first compute whether any actual changes would occur
(compare the incoming payload fields — name, description,
label.LabelMembershipType, and the resolved hostIDs — against the existing label
state and its current membership set obtained from the DS), and if nothing would
change, return early without calling svc.ds.UpdateLabelMembershipByHostIDs,
svc.ds.SaveLabel, or svc.NewActivity; reuse the existing variables (label,
payload, hostIDs, savedHostIDs) and the same filter/context so the check mirrors
ApplyLabelSpecs behavior and only proceeds to
UpdateLabelMembershipByHostIDs/SaveLabel/NewActivity when a real change is
detected.
---
Nitpick comments:
In `@cmd/fleetctl/integrationtest/gitops/gitops_enterprise_integration_test.go`:
- Around line 3382-3539: The test currently only exercises team-scoped dynamic
labels; extend it to cover team-scoped manual and host_vitals/IdP-created label
flows by adding parallel phases that call writeTeam (in addition to writeGlobal)
to create a team manual label (with hosts list) and a team host_vitals label
(with criteria), then run apply() and flush() and assert the emitted activities
include ActivityTypeCreatedLabel, ActivityTypeEditedLabel and
ActivityTypeDeletedLabel for those team labels (verifying FleetID/FleetName
presence for team-scoped activities), and also include no-op re-apply checks and
edits (change query -> change hosts -> change criteria) using the same pattern
as existing global label phases; locate changes around the existing
writeTeam/writeGlobal/apply/flush sequences and reuse the same assertions that
check types (ActivityTypeCreatedLabel, ActivityTypeEditedLabel,
ActivityTypeDeletedLabel) and name membership to ensure team manual and
host_vitals paths are covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0196fe1e-47b0-4d05-b05b-f4de04af5baf
📒 Files selected for processing (4)
cmd/fleetctl/integrationtest/gitops/gitops_enterprise_integration_test.goserver/service/hosts.goserver/service/labels.goserver/service/labels_test.go
nulmete
left a comment
nulmete
left a comment
There was a problem hiding this comment.
Haven't tested locally but code LGTM.
I think the only comment that we might need to address is the one related to "treating no rows as an error, or return an empty slice". The other are stylistic/architectural.
| ID: label.ID, | ||
| Name: label.Name, |
There was a problem hiding this comment.
If I remember correctly, all labels are treated as global as of now, correct?
(So, in the UI we'll never show which fleet this label was created for)
If so, should we put a comment or create a sub-task to pass in the FleetID and FleetName here for future reference?
There was a problem hiding this comment.
Agree, a comment makes sense.
| // labelMatchesScope reports whether the given label belongs to the team scope | ||
| // identified by teamID (nil means global scope). | ||
| func labelMatchesScope(l *fleet.Label, teamID *uint) bool { |
There was a problem hiding this comment.
nit: what about renaming to labelMatchesTeamScope and dropping the comment? I think the function is small enough that we can infer it does that.
same comment for:
- the labelSpecMatchesLabel function below (though the function name and arguments are clear to me)
- the lookupTeamNameForLabel function at the bottom.
There was a problem hiding this comment.
Good idea.
PS: I'll keep comment for labelSpecMatchesLabel to indicate that currently it's through GitOps that you can edit all label fields.
|
|
||
| // lookupTeamNameForLabel returns the team name for the given team ID, or nil | ||
| // if the team ID is nil (global label). | ||
| func (svc *Service) lookupTeamNameForLabel(ctx context.Context, teamID *uint) (*string, error) { |
There was a problem hiding this comment.
🤔 I thought we were passing a label as an argument (since I read "ForLabel" in the function name)
could this function name be simplified to just "lookupTeamName" or "getTeamName"? (we can infer it's for the given teamID that we pass, so no need to include the "For..." suffix IMHO)
| if err := svc.ds.DeleteLabel(ctx, label.Name, filter); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if err := svc.NewActivity(ctx, vc.User, fleet.ActivityTypeDeletedLabel{ | ||
| ID: label.ID, | ||
| Name: label.Name, | ||
| FleetID: label.TeamID, | ||
| FleetName: label.TeamName, | ||
| }); err != nil { | ||
| return ctxerr.Wrap(ctx, err, "create activity for label deletion") | ||
| } | ||
| return nil |
There was a problem hiding this comment.
if the label was deleted but the activity creation failed we'd report an error back to the UI, correct? Not likely to happen, but who knows
anyways, I think this is an existing pattern to create activities right after the main operation of the endpoints (I've seen this in other endpoints as well)
IMHO, creating an activity after something is done is a side effect that should be done asynchronously and shouldn't affect the return status/body of the endpoint
There was a problem hiding this comment.
going to deep here 😅 but in a past project, we had an "operations" layer that was in between the API layer (handler.go) and service layer (this method)... so:
- if we needed to perform ANY async operation, then we would call the operations layer (API -> operations -> service)
- if everything was synchronous, then we'd directly call the service layer (API -> service)
usually operations would involve some enqueuing an async job, so in this case I think we'd enqueue a job to create such activity after the service call to DeleteLabel.
happy to follow-up on this if this is something that would add value in the long run :)
There was a problem hiding this comment.
Agree, this is the current pattern. Needs some revisiting.
| labelNames = server.RemoveDuplicatesFromSlice(labelNames) | ||
| labelIDs, err := svc.validateLabelNames(ctx, "remove", labelNames, tmID) |
There was a problem hiding this comment.
nit: we call validateLabelNames in two different places, so we have to call RemoveDuplicatesFromSlice in both places. Should we consider deduping inside validateLabelNames?
There was a problem hiding this comment.
I actually moved it outside so that the deduped label names are used in the outer scope (issue caught by AI).
Resolves #36976
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Testing
Summary by CodeRabbit
New Features
Tests