👌 Address code review: declarative view diff, simplify isEqual, remove safety fallback

Author: mormubis

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

@@ -1,24 +1,14 @@
+import { isIndexableObject } from '@datadog/browser-core'
+
 /**
  * Compare two values for deep equality
  */
 export function isEqual(a: unknown, b: unknown): boolean {
-  // Reference equality
   if (a === b) {
     return true
   }
 
-  // Handle null/undefined
-  if (a === null || b === null || a === undefined || b === undefined) {
-    return a === b
-  }
-
-  // Type mismatch
-  if (typeof a !== typeof b) {
-    return false
-  }
-
-  // Primitives
-  if (typeof a !== 'object') {
+  if (a === null || typeof a !== 'object' || b === null || typeof b !== 'object') {
     return a === b
   }
 
@@ -30,12 +20,11 @@ export function isEqual(a: unknown, b: unknown): boolean {
     return a.every((val, idx) => isEqual(val, b[idx]))
   }
 
-  // One is array, other is not
   if (Array.isArray(a) || Array.isArray(b)) {
     return false
   }
 
-  // Objects
+  // Plain objects
   const aObj = a as Record<string, unknown>
   const bObj = b as Record<string, unknown>
   const aKeys = Object.keys(aObj)
@@ -54,6 +43,7 @@ export function isEqual(a: unknown, b: unknown): boolean {
 export interface DiffMergeOptions {
   replaceKeys?: Set<string>
   appendKeys?: Set<string>
+  ignoreKeys?: Set<string>
 }
 
 /**
@@ -62,6 +52,7 @@ export interface DiffMergeOptions {
  *
  * Default strategy is REPLACE (isEqual check). Exceptions:
  * - Both values are plain objects and key is not in replaceKeys: recurse (MERGE)
+ *   Sub-paths (e.g. 'view.custom_timings') are propagated to the recursive call.
  * - Both values are arrays and key is in appendKeys: include only new trailing elements (APPEND)
  */
 export function diffMerge(
@@ -72,22 +63,23 @@ export function diffMerge(
   const result: Record<string, unknown> = {}
   const replaceKeys = options?.replaceKeys ?? new Set<string>()
   const appendKeys = options?.appendKeys ?? new Set<string>()
+  const ignoreKeys = options?.ignoreKeys ?? new Set<string>()
 
   for (const key of Object.keys(current)) {
+    if (ignoreKeys.has(key)) {
+      continue
+    }
+
     const currentVal = current[key]
     const lastSentVal = lastSent[key]
 
-    if (
-      !replaceKeys.has(key) &&
-      currentVal !== null &&
-      typeof currentVal === 'object' &&
-      !Array.isArray(currentVal) &&
-      lastSentVal !== null &&
-      typeof lastSentVal === 'object' &&
-      !Array.isArray(lastSentVal)
-    ) {
+    if (!replaceKeys.has(key) && isIndexableObject(currentVal) && isIndexableObject(lastSentVal)) {
       // Both are plain objects and not marked for replace: recurse (MERGE)
-      const nestedDiff = diffMerge(currentVal as Record<string, unknown>, lastSentVal as Record<string, unknown>)
+      const nestedDiff = diffMerge(currentVal, lastSentVal, {
+        replaceKeys: extractSubPaths(replaceKeys, key),
+        appendKeys: extractSubPaths(appendKeys, key),
+        ignoreKeys: extractSubPaths(ignoreKeys, key),
+      })
       if (nestedDiff) {
         result[key] = nestedDiff
       }
@@ -111,3 +103,13 @@ export function diffMerge(
 
   return Object.keys(result).length > 0 ? result : undefined
 }
+
+function extractSubPaths(keys: Set<string>, prefix: string): Set<string> {
+  const result = new Set<string>()
+  for (const key of keys) {
+    if (key.startsWith(`${prefix}.`)) {
+      result.add(key.slice(prefix.length + 1))
+    }
+  }
+  return result
+}

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

@@ -1,5 +1,6 @@
 import type { Observable, RawError, PageMayExitEvent, Encoder } from '@datadog/browser-core'
 import {
+  combine,
   createBatch,
   createFlushController,
   createHttpRequest,
@@ -12,115 +13,58 @@ import type { RumConfiguration } from '../domain/configuration'
 import type { LifeCycle } from '../domain/lifeCycle'
 import { LifeCycleEventType } from '../domain/lifeCycle'
 import type { AssembledRumEvent } from '../rawRumEvent.types'
+import type { RumViewEvent } from '../rumEvent.types'
 import { RumEventType } from '../rawRumEvent.types'
-import { diffMerge, isEqual } from '../domain/view/viewDiff'
+import { diffMerge } from '../domain/view/viewDiff'
 
 export const PARTIAL_VIEW_UPDATE_CHECKPOINT_INTERVAL = 100
 
-// Top-level assembled fields that should be diffed with simple equality
-const ASSEMBLED_TOP_LEVEL_FIELDS = [
-  'service',
-  'version',
-  'source',
-  'ddtags',
-  'context',
-  'connectivity',
-  'usr',
-  'device',
-  'privacy',
-] as const
-
 export function computeAssembledViewDiff(
   current: AssembledRumEvent,
   last: AssembledRumEvent
 ): AssembledRumEvent | undefined {
   const currentObj = current as unknown as Record<string, unknown>
   const lastObj = last as unknown as Record<string, unknown>
 
-  const result: Record<string, unknown> = {
-    type: RumEventType.VIEW_UPDATE,
-    date: currentObj.date,
-    application: currentObj.application,
-    session: currentObj.session,
-  }
-
-  let hasChanges = false
+  const diff = diffMerge(currentObj, lastObj, {
+    // context, connectivity, usr, device, privacy are objects — use REPLACE to avoid partial updates
+    replaceKeys: new Set(['view.custom_timings', 'context', 'connectivity', 'usr', 'device', 'privacy']),
+    appendKeys: new Set(['_dd.page_states']),
+    // Ignore always-required fields — they are added back via combine regardless of changes
+    ignoreKeys: new Set([
+      'date',
+      'type',
+      'application',
+      'session',
+      'view.id',
+      'view.url',
+      '_dd.document_version',
+      '_dd.format_version',
+    ]),
+  })
 
-  // --- view.* diff (MERGE strategy, nested-aware) ---
-  const currentView = currentObj.view as Record<string, unknown>
-  const lastView = lastObj.view as Record<string, unknown>
-  // view.id and view.url are always required by the schema (_common-schema.json) for backend routing
-  const viewResult: Record<string, unknown> = { id: currentView.id, url: currentView.url }
-
-  // Note: diffMerge emits null for keys deleted between events. In practice, view fields only
-  // appear (e.g. first_byte, lcp, cls become available as data arrives) and never disappear
-  // within the same view — so the null-for-deleted-keys path is unreachable for view data.
-  const viewDiff = diffMerge(currentView, lastView, { replaceKeys: new Set(['custom_timings']) })
-  if (viewDiff) {
-    delete viewDiff.id // already in required fields
-    delete viewDiff.url // already in required fields
-    Object.assign(viewResult, viewDiff)
-    if (Object.keys(viewDiff).length > 0) {
-      hasChanges = true
-    }
+  if (!diff) {
+    return undefined
   }
-  result.view = viewResult
 
-  // --- _dd.* diff (MERGE strategy, page_states APPEND) ---
+  const currentView = currentObj.view as Record<string, unknown>
   const currentDd = currentObj._dd as Record<string, unknown>
-  const lastDd = lastObj._dd as Record<string, unknown>
-  // _dd.document_version and _dd.format_version are always required by the schema for backend routing
-  const ddResult: Record<string, unknown> = {
-    document_version: currentDd.document_version,
-    format_version: currentDd.format_version,
-  }
 
-  const ddDiff = diffMerge(currentDd, lastDd, { appendKeys: new Set(['page_states']) })
-  if (ddDiff) {
-    delete ddDiff.document_version // already in required fields
-    delete ddDiff.format_version // already in required fields
-    Object.assign(ddResult, ddDiff)
-    if (Object.keys(ddDiff).length > 0) {
-      hasChanges = true
-    }
-  }
-  result._dd = ddResult
-
-  // --- display.* diff (MERGE strategy) ---
-  const currentDisplay = currentObj.display as Record<string, unknown> | undefined
-  const lastDisplay = lastObj.display as Record<string, unknown> | undefined
-  if (currentDisplay && lastDisplay) {
-    const displayDiff = diffMerge(currentDisplay, lastDisplay)
-    if (displayDiff && Object.keys(displayDiff).length > 0) {
-      result.display = displayDiff
-      hasChanges = true
-    }
-  } else if (currentDisplay && !lastDisplay) {
-    result.display = currentDisplay
-    hasChanges = true
-  } else if (!currentDisplay && lastDisplay) {
-    // In practice this branch is unreachable: display (scroll metrics) only appears
-    // once scroll is tracked and never goes away within the same view. Kept as a
-    // defensive fallback in case the invariant is violated in the future.
-    result.display = null
-    hasChanges = true
-  }
-
-  // --- Top-level assembled fields (REPLACE strategy) ---
-  for (const key of ASSEMBLED_TOP_LEVEL_FIELDS) {
-    const currentVal = currentObj[key]
-    const lastVal = lastObj[key]
-    if (!isEqual(currentVal, lastVal)) {
-      result[key] = currentVal
-      hasChanges = true
-    }
-  }
-
-  if (!hasChanges) {
-    return undefined
-  }
-
-  return result as unknown as AssembledRumEvent
+  // Merge always-required fields on top of the diff for backend routing
+  return combine(diff, {
+    type: RumEventType.VIEW_UPDATE,
+    date: currentObj.date,
+    application: currentObj.application,
+    session: currentObj.session,
+    view: {
+      id: currentView.id,
+      url: currentView.url,
+    },
+    _dd: {
+      document_version: currentDd.document_version,
+      format_version: currentDd.format_version,
+    },
+  }) as unknown as AssembledRumEvent
 }
 
 export function startRumBatch(
@@ -172,7 +116,7 @@ export function startRumBatch(
     }
 
     // View ended (is_active: false)
-    if (!(serverRumEvent.view as any).is_active) {
+    if (!(serverRumEvent as RumViewEvent).view.is_active) {
       lastSentView = undefined
       viewUpdatesSinceCheckpoint = 0
       batch.upsert(serverRumEvent, viewId)
@@ -195,14 +139,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.
-    if (!lastSentView) {
-      // Safety fallback (should not happen in practice)
-      lastSentView = serverRumEvent
-      batch.upsert(serverRumEvent, viewId)
-      return
-    }
-
-    const diff = computeAssembledViewDiff(serverRumEvent, lastSentView)
+    const diff = computeAssembledViewDiff(serverRumEvent, lastSentView!)
     lastSentView = serverRumEvent
     if (diff) {
       sendToExtension('rum', diff)