👌 Address code review feedback

Author: mormubis

packages/rum-core/src/domain/assembly.ts

@@ -50,8 +50,10 @@ export function startRumAssembly(
       ...ROOT_MODIFIABLE_FIELD_PATHS,
     },
     // view_update events are created post-assembly in startRumBatch.ts and never go through
-    // this pipeline — they intentionally bypass beforeSend. This entry is required by the
-    // exhaustive type but is never reached in practice.
+    // this pipeline. The full view event already went through assembly (as RumEventType.VIEW),
+    // so any beforeSend modifications (e.g. PII scrubbing on view.performance.lcp.resource_url)
+    // are already reflected in the view_update diff. This entry is required by the exhaustive
+    // type but is never reached in practice.
     [RumEventType.VIEW_UPDATE]: {},
     [RumEventType.ERROR]: {
       'error.message': 'string',

packages/rum-core/src/domain/view/viewDiff.spec.ts

@@ -28,6 +28,10 @@ describe('isEqual', () => {
     expect(isEqual({ a: 1 }, { b: 1 })).toBe(false)
   })
 
+  it('should return true for objects with same keys in different order', () => {
+    expect(isEqual({ a: 1, b: 2 }, { b: 2, a: 1 })).toBe(true)
+  })
+
   it('should return true for equal arrays', () => {
     expect(isEqual([1, 2, 3], [1, 2, 3])).toBe(true)
   })

packages/rum-core/src/transport/startRumBatch.spec.ts

@@ -3,7 +3,7 @@ import { resetExperimentalFeatures } from '@datadog/browser-core/src/tools/exper
 import { registerCleanupTask } from '@datadog/browser-core/test'
 import type { AssembledRumEvent } from '../rawRumEvent.types'
 import { RumEventType } from '../rawRumEvent.types'
-import { computeAssembledViewDiff, PARTIAL_VIEW_UPDATE_CHECKPOINT_INTERVAL } from './startRumBatch'
+import { assembleViewUpdateEvent, PARTIAL_VIEW_UPDATE_CHECKPOINT_INTERVAL } from './startRumBatch'
 
 function makeAssembledView(overrides: Record<string, unknown> = {}): AssembledRumEvent {
   return {
@@ -38,7 +38,7 @@ function makeAssembledView(overrides: Record<string, unknown> = {}): AssembledRu
   } as unknown as AssembledRumEvent
 }
 
-describe('computeAssembledViewDiff', () => {
+describe('assembleViewUpdateEvent', () => {
   it('should return undefined when nothing has changed', () => {
     const last = makeAssembledView()
     const current = makeAssembledView({
@@ -49,7 +49,7 @@ describe('computeAssembledViewDiff', () => {
         configuration: { start_session_replay_recording_manually: false },
       },
     })
-    const result = computeAssembledViewDiff(current, last)
+    const result = assembleViewUpdateEvent(current, last)
 
     // Only document_version changed (always required, not a "meaningful change")
     // view.* unchanged → should return undefined
@@ -78,7 +78,7 @@ describe('computeAssembledViewDiff', () => {
         time_spent: 100,
       },
     })
-    const result = computeAssembledViewDiff(current, last)!
+    const result = assembleViewUpdateEvent(current, last)!
 
     expect(result.type).toBe(RumEventType.VIEW_UPDATE)
     expect((result as any).application).toEqual({ id: 'app-1' })
@@ -111,7 +111,7 @@ describe('computeAssembledViewDiff', () => {
         time_spent: 5000,
       },
     })
-    const result = computeAssembledViewDiff(current, last)!
+    const result = assembleViewUpdateEvent(current, last)!
 
     expect((result.view as any).action).toEqual({ count: 3 }) // changed
     expect((result.view as any).time_spent).toBe(5000) // changed
@@ -144,7 +144,7 @@ describe('computeAssembledViewDiff', () => {
       service: 'svc',
       version: '1.0.0',
     })
-    const result = computeAssembledViewDiff(current, last)!
+    const result = assembleViewUpdateEvent(current, last)!
 
     expect(result.service).toBeUndefined() // unchanged, stripped
     expect((result as any).version).toBeUndefined() // unchanged, stripped
@@ -173,7 +173,7 @@ describe('computeAssembledViewDiff', () => {
       },
       service: 'new-service',
     })
-    const result = computeAssembledViewDiff(current, last)!
+    const result = assembleViewUpdateEvent(current, last)!
 
     expect(result.service).toBe('new-service')
   })
@@ -201,7 +201,7 @@ describe('computeAssembledViewDiff', () => {
       },
     })
     const currentService = current.service
-    computeAssembledViewDiff(current, last)
+    assembleViewUpdateEvent(current, last)
 
     expect(current.service).toBe(currentService)
   })

packages/rum-core/src/transport/startRumBatch.ts

@@ -19,7 +19,7 @@ import { diffMerge } from '../domain/view/viewDiff'
 
 export const PARTIAL_VIEW_UPDATE_CHECKPOINT_INTERVAL = 100
 
-export function computeAssembledViewDiff(
+export function assembleViewUpdateEvent(
   current: AssembledRumEvent,
   last: AssembledRumEvent
 ): AssembledRumEvent | undefined {
@@ -50,7 +50,7 @@ export function computeAssembledViewDiff(
   const currentView = currentObj.view as Record<string, unknown>
   const currentDd = currentObj._dd as Record<string, unknown>
 
-  // Merge always-required fields on top of the diff for backend routing
+  // Restore the ignoreKeys — backend needs them on every event
   return combine(diff, {
     type: RumEventType.VIEW_UPDATE,
     date: currentObj.date,
@@ -137,7 +137,7 @@ export function startRumBatch(
     // They intentionally bypass RAW_RUM_EVENT_COLLECTED → assembly → RUM_EVENT_COLLECTED, which
     // means they skip beforeSend entirely. view_update is an internal bandwidth optimization —
     // not a customer-visible event type, and not modifiable via beforeSend.
-    const diff = computeAssembledViewDiff(serverRumEvent, lastSentView)
+    const diff = assembleViewUpdateEvent(serverRumEvent, lastSentView)
     lastSentView = serverRumEvent
     if (diff) {
       sendToExtension('rum', diff)

test/e2e/lib/framework/intakeRegistry.ts

@@ -88,6 +88,10 @@ export class IntakeRegistry {
     return this.rumEvents.filter(isRumViewEvent)
   }
 
+  get rumViewUpdateEvents() {
+    return this.rumEvents.filter(isRumViewUpdateEvent)
+  }
+
   get rumVitalEvents() {
     return this.rumEvents.filter(isRumVitalEvent)
   }
@@ -193,6 +197,10 @@ function isRumViewEvent(event: RumEvent): event is RumViewEvent {
   return event.type === 'view'
 }
 
+function isRumViewUpdateEvent(event: RumEvent): boolean {
+  return (event.type as string) === 'view_update'
+}
+
 function isRumErrorEvent(event: RumEvent): event is RumErrorEvent {
   return event.type === 'error'
 }

test/e2e/scenario/rum/partialViewUpdates.scenario.ts

@@ -1,23 +1,5 @@
 import { test, expect } from '@playwright/test'
-import { createTest, html, waitForRequests } from '../../lib/framework'
-import type { IntakeRegistry } from '../../lib/framework'
-
-// Loose type for view_update events received at the intake (no generated schema type yet)
-interface ViewUpdateEvent {
-  type: string
-  date: number
-  application: { id: string }
-  session: { id: string }
-  view: { id: string; is_active?: boolean; [key: string]: unknown }
-  _dd: { document_version: number; [key: string]: unknown }
-  [key: string]: unknown
-}
-
-// Helper: extract view_update events from all RUM events
-// (intakeRegistry.rumViewEvents only returns type==='view')
-function getViewUpdateEvents(intakeRegistry: IntakeRegistry): ViewUpdateEvent[] {
-  return intakeRegistry.rumEvents.filter((e) => (e.type as string) === 'view_update') as unknown as ViewUpdateEvent[]
-}
+import { createTest, waitForRequests } from '../../lib/framework'
 
 test.describe('partial view updates', () => {
   createTest('should send view_update events after the initial view event')
@@ -38,7 +20,7 @@ test.describe('partial view updates', () => {
       expect(viewEvents[0].type).toBe('view')
 
       // Should have at least one view_update
-      const viewUpdateEvents = getViewUpdateEvents(intakeRegistry)
+      const viewUpdateEvents = intakeRegistry.rumViewUpdateEvents
       expect(viewUpdateEvents.length).toBeGreaterThanOrEqual(1)
 
       // All events share the same view.id
@@ -59,20 +41,16 @@ test.describe('partial view updates', () => {
 
       await flushEvents()
 
-      // Collect all view-related events (view + view_update) sorted by document_version
-      const allViewRelatedEvents = [
-        ...intakeRegistry.rumViewEvents.map((e) => ({ _dd: e._dd })),
-        ...getViewUpdateEvents(intakeRegistry).map((e) => ({ _dd: e._dd })),
-      ].sort((a, b) => a._dd.document_version - b._dd.document_version)
+      // Collect document_versions from all view-related events (view + view_update)
+      const allDocVersions = [
+        ...intakeRegistry.rumViewEvents.map((e) => e._dd.document_version),
+        ...intakeRegistry.rumViewUpdateEvents.map((e) => (e._dd as { document_version: number }).document_version),
+      ]
 
-      expect(allViewRelatedEvents.length).toBeGreaterThanOrEqual(2)
+      expect(allDocVersions.length).toBeGreaterThanOrEqual(2)
 
-      // Verify monotonic increase
-      for (let i = 1; i < allViewRelatedEvents.length; i++) {
-        expect(allViewRelatedEvents[i]._dd.document_version).toBeGreaterThan(
-          allViewRelatedEvents[i - 1]._dd.document_version
-        )
-      }
+      // Verify all document_versions are unique (no duplicates)
+      expect(new Set(allDocVersions).size).toBe(allDocVersions.length)
     })
 
   createTest('should only send view events when feature flag is not enabled')
@@ -88,25 +66,16 @@ test.describe('partial view updates', () => {
       expect(intakeRegistry.rumViewEvents.length).toBeGreaterThanOrEqual(1)
 
       // Should NOT have any view_update events
-      const viewUpdateEvents = getViewUpdateEvents(intakeRegistry)
+      const viewUpdateEvents = intakeRegistry.rumViewUpdateEvents
       expect(viewUpdateEvents).toHaveLength(0)
     })
 
   createTest('should emit a new full view event after navigation')
     .withRum({
       enableExperimentalFeatures: ['partial_view_updates'],
     })
-    .withBody(html`
-      <a id="nav-link">Navigate</a>
-      <script>
-        document.getElementById('nav-link').addEventListener('click', () => {
-          history.pushState(null, '', '/new-page')
-        })
-      </script>
-    `)
     .run(async ({ intakeRegistry, flushEvents, page }) => {
-      // Trigger a route change to create a new view
-      await page.click('#nav-link')
+      await page.evaluate(() => history.pushState(null, '', '/new-page'))
 
       await flushEvents()
 
@@ -130,7 +99,7 @@ test.describe('partial view updates', () => {
 
       await flushEvents()
 
-      const viewUpdateEvents = getViewUpdateEvents(intakeRegistry)
+      const viewUpdateEvents = intakeRegistry.rumViewUpdateEvents
       expect(viewUpdateEvents.length).toBeGreaterThanOrEqual(1)
 
       for (const event of viewUpdateEvents) {
@@ -148,17 +117,8 @@ test.describe('partial view updates', () => {
     .withRum({
       enableExperimentalFeatures: ['partial_view_updates'],
     })
-    .withBody(html`
-      <a id="nav-link">Navigate</a>
-      <script>
-        document.getElementById('nav-link').addEventListener('click', () => {
-          history.pushState(null, '', '/other-page')
-        })
-      </script>
-    `)
     .run(async ({ intakeRegistry, flushEvents, page }) => {
-      // Navigate to trigger view end on the first view
-      await page.click('#nav-link')
+      await page.evaluate(() => history.pushState(null, '', '/other-page'))
 
       await flushEvents()
 
@@ -170,7 +130,7 @@ test.describe('partial view updates', () => {
       expect(endEvent?.type).toBe('view')
 
       // No view_update should have is_active: false
-      const viewUpdateEvents = getViewUpdateEvents(intakeRegistry)
+      const viewUpdateEvents = intakeRegistry.rumViewUpdateEvents
       const endUpdateEvent = viewUpdateEvents.find((e) => e.view.id === firstViewId && e.view.is_active === false)
       expect(endUpdateEvent).toBeUndefined()
     })