From ce9900218ef27a91ba1c8ab55ee59a246deb87fe Mon Sep 17 00:00:00 2001 From: rcourtman Date: Tue, 24 Mar 2026 19:46:00 +0000 Subject: [PATCH] Normalize recovery subject labels across rollups --- .../internal/subsystems/storage-recovery.md | 5 ++ .../Recovery/__tests__/Recovery.test.tsx | 44 ++++++++++ frontend-modern/src/types/recovery.ts | 1 + .../recoveryRecordPresentation.test.ts | 36 ++++++++ .../src/utils/recoveryRecordPresentation.ts | 33 +++++--- internal/recovery/model/types.go | 4 + internal/recovery/recovery_test.go | 50 +++++++++++ internal/recovery/rollups.go | 12 +++ internal/recovery/store/store.go | 82 ++++++++++++++++++- internal/recovery/store/store_rollups_test.go | 9 ++ .../canonical_completion_guard_test.py | 18 ++-- 11 files changed, 271 insertions(+), 23 deletions(-) diff --git a/docs/release-control/v6/internal/subsystems/storage-recovery.md b/docs/release-control/v6/internal/subsystems/storage-recovery.md index 73ab56420..99c803405 100644 --- a/docs/release-control/v6/internal/subsystems/storage-recovery.md +++ b/docs/release-control/v6/internal/subsystems/storage-recovery.md @@ -284,6 +284,11 @@ That same direct proof rule also applies to the shared recovery record helper: `frontend-modern/src/utils/recoveryRecordPresentation.ts` must stay on the explicit `recovery-product-surface` proof path instead of inheriting coverage only through pages or higher-level recovery components. +That shared recovery record contract now also includes rollup-side display +payload continuity: the recovery backend must preserve the latest normalized +subject label on rollups, and recovery UI helpers must prefer that canonical +display label before raw subject ids whenever the live unified-resource map is +missing or only resolves to opaque machine identifiers. That same direct proof rule also applies to the shared recovery outcome helper: `frontend-modern/src/utils/recoveryOutcomePresentation.ts` must stay on the explicit `recovery-product-surface` proof path instead of inheriting coverage diff --git a/frontend-modern/src/components/Recovery/__tests__/Recovery.test.tsx b/frontend-modern/src/components/Recovery/__tests__/Recovery.test.tsx index 21174a331..bd2a98912 100644 --- a/frontend-modern/src/components/Recovery/__tests__/Recovery.test.tsx +++ b/frontend-modern/src/components/Recovery/__tests__/Recovery.test.tsx @@ -144,6 +144,50 @@ describe('Recovery', () => { expect(screen.getByText('tank/apps')).toBeInTheDocument(); }); + it('renders canonical rollup and history subject labels when linked resources are unavailable', async () => { + rollupsPayload.push({ + rollupId: 'res:vm-404', + subjectResourceId: 'vm-404', + display: { subjectLabel: 'Archive VM' }, + lastAttemptAt: '2026-02-12T08:00:00.000Z', + lastSuccessAt: '2026-02-12T08:00:00.000Z', + lastOutcome: 'success', + providers: ['proxmox-pve'], + }); + pointsByRollupId['res:vm-404'] = [ + { + id: 'p404', + provider: 'proxmox-pve', + kind: 'backup', + mode: 'local', + outcome: 'success', + completedAt: '2026-02-12T08:00:00.000Z', + display: { subjectLabel: 'Archive VM' }, + }, + ]; + + try { + render(() => ); + + const subject = await screen.findByText('Archive VM'); + fireEvent.click(subject); + + await waitFor(() => { + expect(navigateSpy).toHaveBeenCalledWith('/recovery?rollupId=res%3Avm-404', { + replace: true, + }); + }); + + const tables = await screen.findAllByRole('table'); + const historyTable = tables[tables.length - 1]; + expect(within(historyTable).getByText('Archive VM')).toBeInTheDocument(); + expect(within(historyTable).queryByText('vm-404')).not.toBeInTheDocument(); + } finally { + rollupsPayload.pop(); + delete pointsByRollupId['res:vm-404']; + } + }); + it('focuses history when a rollup is clicked', async () => { render(() => ); diff --git a/frontend-modern/src/types/recovery.ts b/frontend-modern/src/types/recovery.ts index 182148510..e72eb405e 100644 --- a/frontend-modern/src/types/recovery.ts +++ b/frontend-modern/src/types/recovery.ts @@ -75,6 +75,7 @@ export interface ProtectionRollup { rollupId: string; subjectResourceId?: string; subjectRef?: RecoveryExternalRef | null; + display?: RecoveryPointDisplay | null; lastAttemptAt?: string | null; lastSuccessAt?: string | null; diff --git a/frontend-modern/src/utils/__tests__/recoveryRecordPresentation.test.ts b/frontend-modern/src/utils/__tests__/recoveryRecordPresentation.test.ts index a49e97a49..9ff6d906f 100644 --- a/frontend-modern/src/utils/__tests__/recoveryRecordPresentation.test.ts +++ b/frontend-modern/src/utils/__tests__/recoveryRecordPresentation.test.ts @@ -43,6 +43,42 @@ describe('recoveryRecordPresentation', () => { expect(getRecoveryPointDetailsSummary(point)).toBe('Immutable and encrypted'); }); + it('prefers governed display labels over opaque linked-resource ids', () => { + const resources = new Map([ + [ + 'res-2', + { + id: 'res-2', + name: 'res-2', + displayName: 'Payments API', + type: 'vm', + } as Resource, + ], + [ + 'res-3', + { + id: 'res-3', + name: 'res-3', + type: 'vm', + } as Resource, + ], + ]); + + const rollup = { + rollupId: 'res:res-3', + subjectResourceId: 'res-3', + display: { subjectLabel: 'Archive VM' }, + } as ProtectionRollup; + const linkedPoint = { + id: 'point-3', + subjectResourceId: 'res-2', + display: { subjectLabel: 'billing-api' }, + } as RecoveryPoint; + + expect(getRecoveryRollupSubjectLabel(rollup, resources)).toBe('Archive VM'); + expect(getRecoveryPointSubjectLabel(linkedPoint, resources)).toBe('Payments API'); + }); + it('derives timestamps and normalizes mode query values', () => { const point = { completedAt: '2026-03-09T12:00:00.000Z', diff --git a/frontend-modern/src/utils/recoveryRecordPresentation.ts b/frontend-modern/src/utils/recoveryRecordPresentation.ts index 233287ba8..afc7d75fe 100644 --- a/frontend-modern/src/utils/recoveryRecordPresentation.ts +++ b/frontend-modern/src/utils/recoveryRecordPresentation.ts @@ -1,18 +1,31 @@ import type { ProtectionRollup, RecoveryPoint } from '@/types/recovery'; import type { Resource } from '@/types/resource'; +import { getPreferredResourceDisplayName } from '@/utils/resourceIdentity'; export type RecoveryArtifactMode = 'snapshot' | 'local' | 'remote'; +const getRecoveryLinkedResourceLabel = ( + subjectResourceId: string, + resourcesById: Map, +): string => { + if (!subjectResourceId) return ''; + const resource = resourcesById.get(subjectResourceId); + if (!resource) return ''; + const label = getPreferredResourceDisplayName(resource).trim(); + if (!label) return ''; + if (label.toLowerCase() === subjectResourceId.toLowerCase()) return ''; + return label; +}; + export function getRecoveryRollupSubjectLabel( rollup: ProtectionRollup, resourcesById: Map, ): string { const subjectResourceId = (rollup.subjectResourceId || '').trim(); - if (subjectResourceId) { - const resource = resourcesById.get(subjectResourceId); - const name = (resource?.name || '').trim(); - if (name) return name; - } + const displayLabel = String(rollup.display?.subjectLabel || '').trim(); + const linkedResourceLabel = getRecoveryLinkedResourceLabel(subjectResourceId, resourcesById); + if (linkedResourceLabel) return linkedResourceLabel; + if (displayLabel) return displayLabel; const ref = rollup.subjectRef || null; if (ref?.namespace && ref?.name) return `${ref.namespace}/${ref.name}`; @@ -32,14 +45,9 @@ export function getRecoveryPointSubjectLabel( resourcesById: Map, ): string { const subjectResourceId = (point.subjectResourceId || '').trim(); - if (subjectResourceId) { - const resource = resourcesById.get(subjectResourceId); - const name = (resource?.name || '').trim(); - if (name) return name; - return subjectResourceId; - } - const displayLabel = String(point.display?.subjectLabel || '').trim(); + const linkedResourceLabel = getRecoveryLinkedResourceLabel(subjectResourceId, resourcesById); + if (linkedResourceLabel) return linkedResourceLabel; if (displayLabel) return displayLabel; const ref = point.subjectRef || null; @@ -49,6 +57,7 @@ export function getRecoveryPointSubjectLabel( if (name) return name; const id = String(ref?.id || '').trim(); if (id) return id; + if (subjectResourceId) return subjectResourceId; return point.id; } diff --git a/internal/recovery/model/types.go b/internal/recovery/model/types.go index ddaedd22b..27b8ef6ae 100644 --- a/internal/recovery/model/types.go +++ b/internal/recovery/model/types.go @@ -113,6 +113,10 @@ type ProtectionRollup struct { // Latest known subject identity for display in UIs. SubjectResourceID string `json:"subjectResourceId,omitempty"` SubjectRef *ExternalRef `json:"subjectRef,omitempty"` + // Display mirrors the latest normalized subject-facing labels derived from + // the newest point in the rollup so protected-inventory UIs can prefer the + // canonical label contract before falling back to opaque ids. + Display *RecoveryPointDisplay `json:"display,omitempty"` LastAttemptAt *time.Time `json:"lastAttemptAt,omitempty"` LastSuccessAt *time.Time `json:"lastSuccessAt,omitempty"` diff --git a/internal/recovery/recovery_test.go b/internal/recovery/recovery_test.go index a770dcd79..be4039736 100644 --- a/internal/recovery/recovery_test.go +++ b/internal/recovery/recovery_test.go @@ -838,3 +838,53 @@ func TestBuildRollupsFromPoints_SortOrder(t *testing.T) { t.Error("expected rollups sorted by last attempt descending") } } + +func TestBuildRollupsFromPoints_PreservesLatestDisplayLabels(t *testing.T) { + now := time.Now() + laterTime := now.Add(1 * time.Minute) + + points := []RecoveryPoint{ + { + ID: "p1", + Provider: ProviderProxmoxPVE, + SubjectResourceID: "res-1", + StartedAt: &now, + CompletedAt: &now, + Outcome: OutcomeSuccess, + Display: &RecoveryPointDisplay{ + SubjectLabel: "Old Label", + SubjectType: "proxmox-vm", + }, + }, + { + ID: "p2", + Provider: ProviderProxmoxPVE, + SubjectResourceID: "res-1", + StartedAt: &laterTime, + CompletedAt: &laterTime, + Outcome: OutcomeFailed, + Display: &RecoveryPointDisplay{ + SubjectLabel: "Current Label", + SubjectType: "proxmox-vm", + IsWorkload: true, + }, + }, + } + + rollups := BuildRollupsFromPoints(points) + if len(rollups) != 1 { + t.Fatalf("expected 1 rollup, got %d", len(rollups)) + } + if rollups[0].Display == nil { + t.Fatal("expected rollup display to be populated") + } + if got := rollups[0].Display.SubjectLabel; got != "Current Label" { + t.Fatalf("Display.SubjectLabel = %q, want %q", got, "Current Label") + } + if got := rollups[0].Display.SubjectType; got != "proxmox-vm" { + t.Fatalf("Display.SubjectType = %q, want %q", got, "proxmox-vm") + } + if !rollups[0].Display.IsWorkload { + t.Fatal("expected rollup display workload marker to be preserved") + } +} diff --git a/internal/recovery/rollups.go b/internal/recovery/rollups.go index 663586f22..75b68442c 100644 --- a/internal/recovery/rollups.go +++ b/internal/recovery/rollups.go @@ -6,6 +6,14 @@ import ( "time" ) +func displayForRollupPoint(p RecoveryPoint) *RecoveryPointDisplay { + if p.Display != nil { + display := *p.Display + return &display + } + return DeriveIndex(p).ToDisplay() +} + // BuildRollupsFromPoints computes per-subject rollups from a set of recovery points. // This mirrors the sqlite rollup semantics (timestamp selection + success window) // so mock mode and in-memory consumers behave consistently with persisted stores. @@ -28,6 +36,7 @@ func BuildRollupsFromPoints(points []RecoveryPoint) []ProtectionRollup { // Latest identity seen (ties resolved by latestTS/updated/id). subjectRID string subjectRef *ExternalRef + display *RecoveryPointDisplay providers map[Provider]struct{} } @@ -90,6 +99,7 @@ func BuildRollupsFromPoints(points []RecoveryPoint) []ProtectionRollup { lastSuccessMs: 0, subjectRID: strings.TrimSpace(p.SubjectResourceID), subjectRef: p.SubjectRef, + display: displayForRollupPoint(p), providers: make(map[Provider]struct{}, 2), } byKey[subjectKey] = a @@ -119,6 +129,7 @@ func BuildRollupsFromPoints(points []RecoveryPoint) []ProtectionRollup { } a.subjectRID = strings.TrimSpace(p.SubjectResourceID) a.subjectRef = p.SubjectRef + a.display = displayForRollupPoint(p) } } @@ -148,6 +159,7 @@ func BuildRollupsFromPoints(points []RecoveryPoint) []ProtectionRollup { RollupID: strings.TrimSpace(a.subjectKey), SubjectResourceID: a.subjectRID, SubjectRef: a.subjectRef, + Display: a.display, LastAttemptAt: lastAttemptAt, LastSuccessAt: lastSuccessAt, LastOutcome: a.latestOutcome, diff --git a/internal/recovery/store/store.go b/internal/recovery/store/store.go index 767963792..55bd8c106 100644 --- a/internal/recovery/store/store.go +++ b/internal/recovery/store/store.go @@ -1077,6 +1077,15 @@ func (s *Store) ListRollups(ctx context.Context, opts recovery.ListPointsOptions subject_key, subject_resource_id, subject_ref_json, + subject_label, + subject_type, + is_workload, + cluster_label, + node_host_label, + namespace_label, + entity_id_label, + repository_label, + details_summary, provider, outcome, ` + tsExpr + ` AS ts_ms, @@ -1101,12 +1110,33 @@ func (s *Store) ListRollups(ctx context.Context, opts recovery.ListPointsOptions FROM filtered ), latest AS ( - SELECT subject_key, subject_resource_id, subject_ref_json + SELECT + subject_key, + subject_resource_id, + subject_ref_json, + subject_label, + subject_type, + is_workload, + cluster_label, + node_host_label, + namespace_label, + entity_id_label, + repository_label, + details_summary FROM ( SELECT subject_key, subject_resource_id, subject_ref_json, + subject_label, + subject_type, + is_workload, + cluster_label, + node_host_label, + namespace_label, + entity_id_label, + repository_label, + details_summary, ROW_NUMBER() OVER (PARTITION BY subject_key ORDER BY ts_ms DESC, updated_at_ms DESC, id DESC) AS rn FROM filtered ) x @@ -1121,6 +1151,15 @@ func (s *Store) ListRollups(ctx context.Context, opts recovery.ListPointsOptions agg.subject_key, latest.subject_resource_id, latest.subject_ref_json, + latest.subject_label, + latest.subject_type, + latest.is_workload, + latest.cluster_label, + latest.node_host_label, + latest.namespace_label, + latest.entity_id_label, + latest.repository_label, + latest.details_summary, agg.last_attempt_ms, agg.last_success_ms, (SELECT outcome FROM ranked r WHERE r.subject_key = agg.subject_key AND r.rn = 1) AS last_outcome, @@ -1144,12 +1183,38 @@ func (s *Store) ListRollups(ctx context.Context, opts recovery.ListPointsOptions var subjectKey string var subjectRID sql.NullString var subjectRefRaw sql.NullString + var subjectLabel sql.NullString + var subjectType sql.NullString + var isWorkload sql.NullInt64 + var clusterLabel sql.NullString + var nodeHostLabel sql.NullString + var namespaceLabel sql.NullString + var entityIDLabel sql.NullString + var repositoryLabel sql.NullString + var detailsSummary sql.NullString var lastAttemptMs sql.NullInt64 var lastSuccessMs sql.NullInt64 var lastOutcome string var providersRaw sql.NullString - if err := rows.Scan(&subjectKey, &subjectRID, &subjectRefRaw, &lastAttemptMs, &lastSuccessMs, &lastOutcome, &providersRaw); err != nil { + if err := rows.Scan( + &subjectKey, + &subjectRID, + &subjectRefRaw, + &subjectLabel, + &subjectType, + &isWorkload, + &clusterLabel, + &nodeHostLabel, + &namespaceLabel, + &entityIDLabel, + &repositoryLabel, + &detailsSummary, + &lastAttemptMs, + &lastSuccessMs, + &lastOutcome, + &providersRaw, + ); err != nil { return nil, 0, err } @@ -1189,10 +1254,23 @@ func (s *Store) ListRollups(ctx context.Context, opts recovery.ListPointsOptions outcome = recovery.OutcomeUnknown } + display := recovery.PointIndex{ + SubjectLabel: strings.TrimSpace(subjectLabel.String), + SubjectType: strings.TrimSpace(subjectType.String), + IsWorkload: isWorkload.Valid && isWorkload.Int64 != 0, + ClusterLabel: strings.TrimSpace(clusterLabel.String), + NodeHostLabel: strings.TrimSpace(nodeHostLabel.String), + NamespaceLabel: strings.TrimSpace(namespaceLabel.String), + EntityIDLabel: strings.TrimSpace(entityIDLabel.String), + RepositoryLabel: strings.TrimSpace(repositoryLabel.String), + DetailsSummary: strings.TrimSpace(detailsSummary.String), + }.ToDisplay() + out = append(out, recovery.ProtectionRollup{ RollupID: strings.TrimSpace(subjectKey), SubjectResourceID: strings.TrimSpace(subjectRID.String), SubjectRef: subjectRefPtr, + Display: display, LastAttemptAt: lastAttemptAt, LastSuccessAt: lastSuccessAt, LastOutcome: outcome, diff --git a/internal/recovery/store/store_rollups_test.go b/internal/recovery/store/store_rollups_test.go index d213def80..a5be75001 100644 --- a/internal/recovery/store/store_rollups_test.go +++ b/internal/recovery/store/store_rollups_test.go @@ -93,6 +93,9 @@ func TestStore_ListRollups(t *testing.T) { if got[0].SubjectRef == nil || got[0].SubjectRef.Type != "truenas-dataset" || got[0].SubjectRef.Name != "tank/apps" { t.Fatalf("rollup[0].SubjectRef = %#v, want truenas dataset ref", got[0].SubjectRef) } + if got[0].Display == nil || got[0].Display.SubjectLabel != "tank/apps" { + t.Fatalf("rollup[0].Display = %#v, want subject label tank/apps", got[0].Display) + } // Second: vm-1 with latest failure at t2 and last success at t1. if got[1].RollupID != "res:vm-1" { @@ -104,6 +107,9 @@ func TestStore_ListRollups(t *testing.T) { if got[1].SubjectRef != nil { t.Fatalf("rollup[1].SubjectRef = %#v, want nil", got[1].SubjectRef) } + if got[1].Display == nil || got[1].Display.SubjectLabel != "vm-1" { + t.Fatalf("rollup[1].Display = %#v, want subject label vm-1", got[1].Display) + } if got[1].LastAttemptAt == nil || !got[1].LastAttemptAt.Equal(t2) { t.Fatalf("rollup[1].LastAttemptAt = %v, want %v", got[1].LastAttemptAt, t2) } @@ -196,4 +202,7 @@ func TestStore_ListRollups(t *testing.T) { if got3[0].RollupID != "res:pod-1" { t.Fatalf("rollup with normalized filters = %q, want %q", got3[0].RollupID, "res:pod-1") } + if got3[0].Display == nil || got3[0].Display.SubjectLabel != "default/pod-1" { + t.Fatalf("rollup with normalized filters display = %#v, want subject label default/pod-1", got3[0].Display) + } } diff --git a/scripts/release_control/canonical_completion_guard_test.py b/scripts/release_control/canonical_completion_guard_test.py index cac792ade..b997ca789 100644 --- a/scripts/release_control/canonical_completion_guard_test.py +++ b/scripts/release_control/canonical_completion_guard_test.py @@ -3035,17 +3035,21 @@ index 1111111..2222222 100644 "allow_same_subsystem_tests": False, "test_prefixes": [], "exact_files": [ + "frontend-modern/src/components/Dashboard/GuestDrawer.test.tsx", + "frontend-modern/src/components/Dashboard/MetricBar.test.tsx", + "frontend-modern/src/components/Dashboard/StackedMemoryBar.test.tsx", + "frontend-modern/src/components/Dashboard/ThresholdSlider.test.tsx", "frontend-modern/src/components/Dashboard/__tests__/Dashboard.performance.contract.test.tsx", "frontend-modern/src/components/Dashboard/__tests__/DashboardFilter.test.tsx", + "frontend-modern/src/components/Dashboard/__tests__/DiskList.test.tsx", + "frontend-modern/src/components/Dashboard/__tests__/EnhancedCPUBar.test.tsx", + "frontend-modern/src/components/Dashboard/__tests__/GuestRow.test.tsx", + "frontend-modern/src/components/Dashboard/__tests__/StackedDiskBar.test.tsx", "frontend-modern/src/components/Dashboard/__tests__/dashboardSelectionModel.test.ts", "frontend-modern/src/components/Dashboard/__tests__/dashboardWorkloadFilterConfigModel.test.ts", "frontend-modern/src/components/Dashboard/__tests__/dashboardWorkloadRouteModel.test.ts", "frontend-modern/src/components/Dashboard/__tests__/dashboardWorkloadRouteStateModel.test.ts", "frontend-modern/src/components/Dashboard/__tests__/dashboardWorkloadUrlSyncModel.test.ts", - "frontend-modern/src/components/Dashboard/__tests__/DiskList.test.tsx", - "frontend-modern/src/components/Dashboard/__tests__/EnhancedCPUBar.test.tsx", - "frontend-modern/src/components/Dashboard/__tests__/GuestRow.test.tsx", - "frontend-modern/src/components/Dashboard/__tests__/StackedDiskBar.test.tsx", "frontend-modern/src/components/Dashboard/__tests__/useDashboardFilterState.test.ts", "frontend-modern/src/components/Dashboard/__tests__/useDashboardSelectionState.test.ts", "frontend-modern/src/components/Dashboard/__tests__/useDashboardWorkloadViewportSync.test.tsx", @@ -3057,12 +3061,8 @@ index 1111111..2222222 100644 "frontend-modern/src/components/Dashboard/__tests__/useThresholdSliderState.test.ts", "frontend-modern/src/components/Dashboard/__tests__/workloadSelectors.test.ts", "frontend-modern/src/components/Dashboard/__tests__/workloadTopology.test.ts", - "frontend-modern/src/components/Dashboard/GuestDrawer.test.tsx", - "frontend-modern/src/components/Dashboard/MetricBar.test.tsx", - "frontend-modern/src/components/Dashboard/StackedMemoryBar.test.tsx", - "frontend-modern/src/components/Dashboard/ThresholdSlider.test.tsx", - "frontend-modern/src/components/Infrastructure/__tests__/infrastructureSummaryModel.test.ts", "frontend-modern/src/components/Infrastructure/__tests__/UnifiedResourceTable.performance.contract.test.tsx", + "frontend-modern/src/components/Infrastructure/__tests__/infrastructureSummaryModel.test.ts", "frontend-modern/src/components/Infrastructure/__tests__/unifiedResourceTableStateModel.test.ts", "frontend-modern/src/components/Workloads/WorkloadsSummary.test.tsx", "frontend-modern/src/utils/__tests__/thresholdSliderPresentation.test.ts",