Open
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.79% +0.02%
==========================================
Files 2636 2637 +1
Lines 211834 212348 +514
Branches 9425 9449 +24
==========================================
+ Hits 141437 141833 +396
- Misses 57551 57617 +66
- Partials 12846 12898 +52
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
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}
Contributor
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
Contributor
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.
Contributor
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
Contributor
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
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
nulmete
approved these changes
May 1, 2026
Member
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.
| const fleetText = activity.details?.fleet_name ? ( | ||
| <> | ||
| {" "} | ||
| on the <b>{activity.details.fleet_name}</b> fleet |
Member
There was a problem hiding this comment.
Figma doesn't show the Fleet that the label was deleted from, but I think it's worth showing so I'd keep this 👍
Comment on lines
+749
to
+752
| if err := sqlx.SelectContext(ctx, ds.reader(ctx), &hostIDs, | ||
| `SELECT host_id FROM label_membership WHERE label_id = ?`, labelID); err != nil { | ||
| return nil, ctxerr.Wrap(ctx, err, "selecting label membership host IDs") | ||
| } |
Member
There was a problem hiding this comment.
should we treat no rows as an error, or return an empty slice instead?
Comment on lines
+3632
to
+3634
| if len(labelNames) == 0 { | ||
| return nil | ||
| } |
Member
There was a problem hiding this comment.
I see that both callers already check len(labelIDs) not being 0 before calling this function, so maybe we can remove this defensive check?
Comment on lines
+3650
to
+3651
| if l.TeamID != nil { | ||
| teamName, err = svc.lookupTeamNameForLabel(ctx, l.TeamID) |
Member
There was a problem hiding this comment.
nit: maybe we could have a single query that combines what LabelsByName does and what this one does, so that we have a single query with a JOIN instead of having two queries (and this one being in a loop).
That way, we get "everything we need" before iterating labels and then emitting the activities.
I think the main challenge would be to add the "TeamName" to the fleet.Label struct, since I think we don't distinguish between DB types and fleet/service types, right?
Member
There was a problem hiding this comment.
Ok I looked at the edited label activity created in labels.go (L218-232), I see that labelDB is called and we already have a LabelWithTeamName struct... so perhaps we could reuse some of that?
Comment on lines
+114
to
+115
| ID: label.ID, | ||
| Name: label.Name, |
Member
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?
Comment on lines
+805
to
+807
| // 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 { |
Member
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.
|
|
||
| // 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) { |
Member
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)
Comment on lines
+570
to
+582
| 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 |
Member
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
Member
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 :)
Comment on lines
+3612
to
3613
| labelNames = server.RemoveDuplicatesFromSlice(labelNames) | ||
| labelIDs, err := svc.validateLabelNames(ctx, "remove", labelNames, tmID) |
Member
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?
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.
Resolves #36976
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Testing
Summary by CodeRabbit
New Features
Tests