Skip to content

refactor(featurelistdiff): Differentiate between removed and deleted features #2146

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jcscottiii merged 1 commit into from
Jan 14, 2026
Merged

refactor(featurelistdiff): Differentiate between removed and deleted features #2146

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 14 additions & 5 deletions lib/blobtypes/featurelistdiff/v1/reconciler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,23 +38,28 @@ func (w *FeatureDiffWorkflow) ReconcileHistory(ctx context.Context) error {

// Phase 1: Investigation
// Iterate through all removed features to build a map of their historical outcomes.
for i := range w.diff.Removed {
r := &w.diff.Removed[i]

remainingRemoved := make([]FeatureRemoved, 0, len(w.diff.Removed))
for _, r := range w.diff.Removed {
// Check the current status of the removed feature ID in the database.
result, err := w.fetcher.GetFeature(ctx, r.ID)
if err != nil {
// If the entity is completely gone from the DB, it's a true deletion.
// We update the reason to allow for specific UI messaging (e.g. "Deleted from platform").
if errors.Is(err, backendtypes.ErrEntityDoesNotExist) {
r.Reason = ReasonDeleted
w.diff.Deleted = append(w.diff.Deleted, FeatureDeleted{
ID: r.ID,
Name: r.Name,
Reason: ReasonDeleted,
})

continue
}

return err
}

// If the feature was not deleted, keep it in the list to check for moves/splits.
remainingRemoved = append(remainingRemoved, r)

// Update the visitor context so it knows which OldID owns the result we are about to visit.
visitor.currentID = r.ID

Expand All @@ -63,6 +68,10 @@ func (w *FeatureDiffWorkflow) ReconcileHistory(ctx context.Context) error {
return err
}
}
// Update the diff with the (now smaller) list of removed items to check.
if len(remainingRemoved) > 0 {
w.diff.Removed = remainingRemoved
}

// Phase 2: Correlation
// If we found any history records, try to match them with the 'Added' list.
Expand Down
67 changes: 64 additions & 3 deletions lib/blobtypes/featurelistdiff/v1/reconciler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ func TestReconcileHistory(t *testing.T) {
name: "Scenario 1: Feature Moved (Rename)",
initialDiff: &FeatureDiff{
Removed: []FeatureRemoved{{ID: "old-id", Name: "Old Name", Reason: ReasonUnmatched}},
Deleted: nil,
Added: []FeatureAdded{{ID: "new-id", Name: "New Name", Reason: ReasonNewMatch, Docs: nil}},
QueryChanged: false,
Modified: nil,
Expand All @@ -84,6 +85,7 @@ func TestReconcileHistory(t *testing.T) {
QueryChanged: false,
Modified: nil,
Splits: nil,
Deleted: nil,
},
wantErr: false,
},
Expand All @@ -99,6 +101,7 @@ func TestReconcileHistory(t *testing.T) {
Modified: nil,
Moves: nil,
Splits: nil,
Deleted: nil,
},
mockResults: map[string]*backendtypes.GetFeatureResult{
"monolith": backendtypes.NewGetFeatureResult(
Expand Down Expand Up @@ -127,6 +130,7 @@ func TestReconcileHistory(t *testing.T) {
QueryChanged: false,
Modified: nil,
Moves: nil,
Deleted: nil,
},
wantErr: false,
},
Expand All @@ -142,6 +146,7 @@ func TestReconcileHistory(t *testing.T) {
Modified: nil,
Moves: nil,
Splits: nil,
Deleted: nil,
},
mockResults: map[string]*backendtypes.GetFeatureResult{
"monolith": backendtypes.NewGetFeatureResult(
Expand Down Expand Up @@ -170,6 +175,7 @@ func TestReconcileHistory(t *testing.T) {
QueryChanged: false,
Modified: nil,
Moves: nil,
Deleted: nil,
},
wantErr: false,
},
Expand All @@ -182,6 +188,7 @@ func TestReconcileHistory(t *testing.T) {
Modified: nil,
Moves: nil,
Splits: nil,
Deleted: nil,
},
mockResults: map[string]*backendtypes.GetFeatureResult{
"removed-id": backendtypes.NewGetFeatureResult(
Expand All @@ -208,26 +215,29 @@ func TestReconcileHistory(t *testing.T) {
Modified: nil,
Moves: nil,
Splits: nil,
Deleted: nil,
},
wantErr: false,
},
{
name: "Scenario 5: Hard Delete (EntityDoesNotExist)",
initialDiff: &FeatureDiff{
Removed: []FeatureRemoved{{ID: "deleted-id", Name: "Deleted Feature", Reason: ReasonUnmatched}},
Deleted: []FeatureDeleted{{ID: "deleted-id", Name: "Deleted Feature", Reason: ReasonDeleted}},
Added: nil,
QueryChanged: false,
Modified: nil,
Moves: nil,
Splits: nil,
Removed: nil,
},
mockResults: nil,
mockErrors: map[string]error{
"deleted-id": backendtypes.ErrEntityDoesNotExist,
},
expectedDiff: &FeatureDiff{
// Remains in Removed list, but Reason updated to Deleted
Removed: []FeatureRemoved{{ID: "deleted-id", Name: "Deleted Feature", Reason: ReasonDeleted}},
// Should be moved to Deleted list
Removed: nil,
Deleted: []FeatureDeleted{{ID: "deleted-id", Name: "Deleted Feature", Reason: ReasonDeleted}},
Added: nil,
QueryChanged: false,
Modified: nil,
Expand All @@ -247,6 +257,7 @@ func TestReconcileHistory(t *testing.T) {
Modified: nil,
Moves: nil,
Splits: nil,
Deleted: nil,
},
mockResults: map[string]*backendtypes.GetFeatureResult{
"old-id": backendtypes.NewGetFeatureResult(
Expand All @@ -261,6 +272,7 @@ func TestReconcileHistory(t *testing.T) {
Modified: nil,
Moves: nil,
Splits: nil,
Deleted: nil,
},
wantErr: false,
},
Expand All @@ -273,6 +285,7 @@ func TestReconcileHistory(t *testing.T) {
Modified: nil,
Moves: nil,
Splits: nil,
Deleted: nil,
},
mockResults: nil,
mockErrors: map[string]error{
Expand All @@ -292,6 +305,7 @@ func TestReconcileHistory(t *testing.T) {
Modified: nil,
Moves: nil,
Splits: nil,
Deleted: nil,
},
mockResults: map[string]*backendtypes.GetFeatureResult{
"monolith": backendtypes.NewGetFeatureResult(
Expand All @@ -310,6 +324,7 @@ func TestReconcileHistory(t *testing.T) {
Modified: nil,
Moves: nil,
Splits: nil,
Deleted: nil,
},
wantErr: false,
},
Expand All @@ -327,6 +342,7 @@ func TestReconcileHistory(t *testing.T) {
Modified: nil,
Moves: nil,
Splits: nil,
Deleted: nil,
},
mockResults: map[string]*backendtypes.GetFeatureResult{
"old-id": backendtypes.NewGetFeatureResult(
Expand All @@ -343,6 +359,51 @@ func TestReconcileHistory(t *testing.T) {
QueryChanged: false,
Modified: nil,
Splits: nil,
Deleted: nil,
},
wantErr: false,
},
{
name: "Scenario 10: Mixed Removed and Deleted",
initialDiff: &FeatureDiff{
Removed: []FeatureRemoved{
{ID: "deleted-id", Name: "Deleted Feature", Reason: ReasonUnmatched},
{ID: "removed-id", Name: "Removed Feature", Reason: ReasonUnmatched},
},
Added: nil,
QueryChanged: false,
Modified: nil,
Moves: nil,
Splits: nil,
Deleted: nil,
},
mockResults: map[string]*backendtypes.GetFeatureResult{
"removed-id": backendtypes.NewGetFeatureResult(
backendtypes.NewRegularFeatureResult(&backend.Feature{
FeatureId: "removed-id",
Name: "",
Spec: nil,
Baseline: nil,
BrowserImplementations: nil,
Discouraged: nil,
Usage: nil,
Wpt: nil,
VendorPositions: nil,
DeveloperSignals: nil,
}),
),
},
mockErrors: map[string]error{
"deleted-id": backendtypes.ErrEntityDoesNotExist,
},
expectedDiff: &FeatureDiff{
Removed: []FeatureRemoved{{ID: "removed-id", Name: "Removed Feature", Reason: ReasonUnmatched}},
Deleted: []FeatureDeleted{{ID: "deleted-id", Name: "Deleted Feature", Reason: ReasonDeleted}},
Added: nil,
QueryChanged: false,
Modified: nil,
Moves: nil,
Splits: nil,
},
wantErr: false,
},
Expand Down
16 changes: 15 additions & 1 deletion lib/blobtypes/featurelistdiff/v1/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ type FeatureDiff struct {
QueryChanged bool `json:"queryChanged,omitempty"`
Added []FeatureAdded `json:"added,omitempty"`
Removed []FeatureRemoved `json:"removed,omitempty"`
Deleted []FeatureDeleted `json:"deleted,omitempty"`
Modified []FeatureModified `json:"modified,omitempty"`
Moves []FeatureMoved `json:"moves,omitempty"`
Splits []FeatureSplit `json:"splits,omitempty"`
Expand Down Expand Up @@ -125,6 +126,13 @@ func (d *FeatureDiff) Sort() {

return d.Removed[i].ID < d.Removed[j].ID
})
sort.Slice(d.Deleted, func(i, j int) bool {
if d.Deleted[i].Name != d.Deleted[j].Name {
return d.Deleted[i].Name < d.Deleted[j].Name
}

return d.Deleted[i].ID < d.Deleted[j].ID
})
sort.Slice(d.Modified, func(i, j int) bool {
if d.Modified[i].Name != d.Modified[j].Name {
return d.Modified[i].Name < d.Modified[j].Name
Expand Down Expand Up @@ -182,6 +190,12 @@ type FeatureRemoved struct {
Reason ChangeReason `json:"reason"`
}

type FeatureDeleted struct {
ID string `json:"id"`
Name string `json:"name"`
Reason ChangeReason `json:"reason"`
}

type FeatureMoved struct {
FromID string `json:"fromId"`
ToID string `json:"toId"`
Expand Down Expand Up @@ -223,7 +237,7 @@ func (d FeatureDiff) HasChanges() bool {
}

func (d FeatureDiff) HasDataChanges() bool {
return len(d.Added) > 0 || len(d.Removed) > 0 ||
return len(d.Added) > 0 || len(d.Removed) > 0 || len(d.Deleted) > 0 ||
len(d.Modified) > 0 || len(d.Moves) > 0 || len(d.Splits) > 0
}

Expand Down
35 changes: 33 additions & 2 deletions lib/blobtypes/featurelistdiff/v1/types_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ func TestHasChanges(t *testing.T) {
Modified: nil,
Moves: nil,
Splits: nil,
Deleted: nil,
},
expected: false,
},
Expand All @@ -48,6 +49,7 @@ func TestHasChanges(t *testing.T) {
Modified: nil,
Moves: nil,
Splits: nil,
Deleted: nil,
},
expected: true,
},
Expand All @@ -60,6 +62,7 @@ func TestHasChanges(t *testing.T) {
Modified: nil,
Moves: nil,
Splits: nil,
Deleted: nil,
},
expected: true,
},
Expand All @@ -72,6 +75,22 @@ func TestHasChanges(t *testing.T) {
Modified: nil,
Moves: nil,
Splits: nil,
Deleted: nil,
},
expected: true,
},
{
name: "Deleted",
diff: FeatureDiff{
QueryChanged: false,
Added: nil,
Removed: nil,
Modified: nil,
Moves: nil,
Splits: nil,
Deleted: []FeatureDeleted{
{ID: "1", Name: "A", Reason: ReasonDeleted},
},
},
expected: true,
},
Expand Down Expand Up @@ -101,8 +120,9 @@ func TestHasChanges(t *testing.T) {
BrowserChanges: nil,
DocsChange: nil,
}},
Moves: nil,
Splits: nil,
Moves: nil,
Splits: nil,
Deleted: nil,
},
expected: true,
},
Expand All @@ -115,6 +135,7 @@ func TestHasChanges(t *testing.T) {
Modified: nil,
Moves: []FeatureMoved{{FromID: "A", ToID: "B", FromName: "A", ToName: "B"}},
Splits: nil,
Deleted: nil,
},
expected: true,
},
Expand All @@ -127,6 +148,7 @@ func TestHasChanges(t *testing.T) {
Modified: nil,
Moves: nil,
Splits: []FeatureSplit{{FromID: "A", FromName: "A", To: nil}},
Deleted: nil,
},
expected: true,
},
Expand Down Expand Up @@ -176,6 +198,10 @@ func TestFeatureDiff_Sort(t *testing.T) {
To: nil,
},
},
Deleted: []FeatureDeleted{
{ID: "2", Name: "B", Reason: ReasonDeleted},
{ID: "1", Name: "A", Reason: ReasonDeleted},
},
}

diff.Sort()
Expand All @@ -190,6 +216,11 @@ func TestFeatureDiff_Sort(t *testing.T) {
t.Errorf("Removed sort failed: %+v", diff.Removed)
}

// Deleted: A(1), B(2)
if diff.Deleted[0].ID != "1" || diff.Deleted[1].ID != "2" {
t.Errorf("Deleted sort failed: %+v", diff.Deleted)
}

// Modified: A(1), B(2)
if diff.Modified[0].ID != "1" || diff.Modified[1].ID != "2" {
t.Errorf("Modified sort failed: %+v", diff.Modified)
Expand Down
Loading
Loading