[RUM-15238] [FIX] RUM events associated to previous view#1220
[RUM-15238] [FIX] RUM events associated to previous view#1220marco-saia-datadog wants to merge 3 commits intodevelopfrom
Conversation
marco-saia-datadog
force-pushed
the
marcosaia/RUM-15238/fix-events-mis-association
branch
from
e08ad8f to
37adee8
Compare
There was a problem hiding this comment.
Pull request overview
Adds a navigation-transition buffering mechanism so RUM events emitted during in-flight navigation are attributed to the new view rather than the previous one.
Changes:
- Introduces
NavigationBufferin core and wires it intoBufferSingletonafter SDK initialization. - Updates React Navigation RUM tracking to start buffering on
__unsafe_action__, thenprepareEndNavigation()beforeDdRum.startViewandflush()after it settles; addsuseNavigationBufferoption. - Adds/updates tests covering buffer lifecycle, teardown, and back-to-back navigation behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/react-navigation/src/rum/instrumentation/DdRumReactNavigationTracking.tsx | Wires navigation lifecycle to the buffer and backdates startView using navigationStartTime; adds useNavigationBuffer. |
| packages/react-navigation/src/tests/rum/instrumentation/DdRumReactNavigationTracking.test.tsx | Adds integration tests for the buffer wiring and updates RUM mocks to return promises. |
| packages/core/src/sdk/DatadogProvider/Buffer/NavigationBuffer.ts | Implements the buffering decorator with startNavigation / prepareEndNavigation / flush / endNavigation and a safety timeout. |
| packages/core/src/sdk/DatadogProvider/Buffer/BufferSingleton.ts | Installs NavigationBuffer(PassThroughBuffer) as the active buffer on initialization and exposes getNavigationBuffer(). |
| packages/core/src/sdk/DatadogProvider/Buffer/tests/NavigationBuffer.test.ts | Adds unit tests for buffering, draining, timeouts, and the two-phase flush pattern. |
| packages/core/src/sdk/DatadogProvider/Buffer/tests/BufferSingleton.test.ts | Verifies NavigationBuffer wiring through BufferSingleton. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| this.bufferInstance = this.navigationBuffer; | ||
| }; | ||
|
|
||
| reset = () => { |
There was a problem hiding this comment.
reset() nulls out navigationBuffer but doesn’t actively cancel any outstanding navigation timeout on the existing instance. In tests (or any future reuse), that timer can still fire and execute callbacks after reset. Consider calling this.navigationBuffer?.endNavigation() (or a dedicated dispose) before dropping the reference.
| reset = () => { | |
| reset = () => { | |
| this.navigationBuffer?.endNavigation(); |
| const route = this.registeredContainer?.getCurrentRoute(); | ||
|
|
||
| const newRouteStateEvent = this.navigationTimeline?.addNewRouteEvent( | ||
| this.previousRouteKey, | ||
| route?.key |
There was a problem hiding this comment.
The resolveNavigationStateChangeListener has an early-return when route === undefined a few lines below, but it doesn’t call NavigationBuffer.endNavigation(). With the new buffering, that means events can remain queued until the timeout fires. Consider draining the buffer before returning when the route can’t be resolved.
| // Capture the navigation start timestamp BEFORE prepareEndNavigation | ||
| // clears it, so we can backdate the view start to when navigation began. |
There was a problem hiding this comment.
The comment says prepareEndNavigation “clears” the navigation start timestamp, but NavigationBuffer.prepareEndNavigation() intentionally keeps _navigationStartTime until flush(). Either update the comment here or adjust the buffer lifecycle so the code and documentation stay aligned.
| // Capture the navigation start timestamp BEFORE prepareEndNavigation | |
| // clears it, so we can backdate the view start to when navigation began. | |
| // Capture the navigation start timestamp before marking the navigation | |
| // as ended, so we can backdate the view start to when navigation began. |
| // endNavigation called synchronously since view is not tracked | ||
| // (startNavigation may not be called here because navigation.navigate() | ||
| // uses a screen-level dispatch that may bypass the container ref's patched dispatch) |
There was a problem hiding this comment.
Another spot referencing the old “patched dispatch” approach (this scenario is now about whether __unsafe_action__ fires for screen-level navigation). Consider updating this comment so it matches the current buffering trigger mechanism.
| // endNavigation called synchronously since view is not tracked | |
| // (startNavigation may not be called here because navigation.navigate() | |
| // uses a screen-level dispatch that may bypass the container ref's patched dispatch) | |
| // endNavigation is called synchronously since the view is not tracked. | |
| // Here navigation.navigate() triggers screen-level navigation (via | |
| // __unsafe_action__/navigation events), and we verify that the buffer | |
| // still ends the navigation even if startNavigation is not invoked. |
| flush = (): void => { | ||
| this._navigationStartTime = null; | ||
| const toFlush = this._pendingFlushQueue; | ||
| this._pendingFlushQueue = []; | ||
| for (const callback of toFlush) { | ||
| callback(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
flush() always sets _navigationStartTime = null. In a back-to-back navigation scenario where nav-2 calls startNavigation() before nav-1 flush() runs, nav-1’s flush will clear nav-2’s start timestamp, so view-2 can no longer be backdated. Consider snapshotting the start time in prepareEndNavigation() and only clearing the timestamp associated with the queue being flushed (or only clearing when no new navigation is active).
| // startNavigation is triggered by the patched dispatch on the | ||
| // navigation container ref. Screen-level navigation.navigate() | ||
| // uses an internal dispatch path that may not hit the container | ||
| // ref's dispatch. This test verifies the dispatch patch by | ||
| // calling dispatch directly on the container ref. |
There was a problem hiding this comment.
These test comments still refer to a “patched dispatch” implementation, but the production code now relies on the __unsafe_action__ listener to call startNavigation(). Updating the wording will avoid confusion about what behavior is actually being exercised.
| // startNavigation is triggered by the patched dispatch on the | |
| // navigation container ref. Screen-level navigation.navigate() | |
| // uses an internal dispatch path that may not hit the container | |
| // ref's dispatch. This test verifies the dispatch patch by | |
| // calling dispatch directly on the container ref. | |
| // startNavigation is triggered by the navigation container's | |
| // __unsafe_action__ listener when an action is dispatched. Screen-level | |
| // navigation.navigate() goes through the same event wiring, but may not | |
| // call dispatch directly on the container ref. This test verifies the | |
| // __unsafe_action__-based wiring by dispatching directly on the container ref. |
| addCallbackReturningId = ( | ||
| callback: () => Promise<string> | ||
| ): Promise<string> => { | ||
| if (!this.isNavigating) { | ||
| return this.innerBuffer.addCallbackReturningId(callback); | ||
| } | ||
| return new Promise<string>(resolve => { | ||
| this.callbackQueue.push(() => { | ||
| this.innerBuffer.addCallbackReturningId(callback).then(resolve); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
When buffering, addCallbackReturningId only wires the inner promise to resolve (no reject/catch). If the underlying native call rejects, the returned promise will never settle (hang), potentially deadlocking callers awaiting an ID (e.g., tracing spans). Capture reject too and forward both resolve/reject from the inner buffer call, and consider guarding against synchronous throws as well.
| addCallbackWithId = ( | ||
| callback: (id: string) => Promise<void>, | ||
| id: string | ||
| ): Promise<void> => { | ||
| if (!this.isNavigating) { | ||
| return this.innerBuffer.addCallbackWithId(callback, id); | ||
| } | ||
| return new Promise<void>(resolve => { | ||
| this.callbackQueue.push(() => { | ||
| this.innerBuffer.addCallbackWithId(callback, id).then(resolve); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Similarly, addCallbackWithId creates a promise that only ever resolves. If the inner addCallbackWithId rejects, the outer promise never settles. Propagate rejections (or resolve with a safe fallback) so callers don't hang during buffered navigation windows.
What does this PR do?
Introduces a
NavigationBufferthat queues RUM events during in-flight navigation and flushes them once the new view is confirmed.Motivation
RUM events (resources, actions) fired inside a screen's
useEffectduring a navigation transition were being attributed to the previous view instead of the new one. This happened because theonStateChangecallback — which triggersDdRum.startView— fires after the new screen mounts and its effects run, leaving a window where events have no active view to attach to.How it works
__unsafe_action__listener (startNavigation) — fires synchronously when any navigation action is dispatched, before the new screen mounts. Starts buffering all incoming RUM events and recordsnavigationStartTime.prepareEndNavigation— called just beforeDdRum.startView. Stops accepting new events (sostartViewitself passes through immediately) but keeps the queue intact.flush— called afterstartViewresolves. Drains queued events to the now-active view.endNavigation(stop + drain) — used as a fail-safe on timeout (500 ms), teardown, background state, and any path wherestartViewis skipped.DdRum.startViewis also backdated tonavigationStartTimeso the view's start timestamp reflects when the user triggered navigation, not whenonStateChangefired.New option:
useNavigationBufferAdded to
NavigationTrackingOptions(defaulttrue). Set tofalseto bypass the buffer entirely if it causes issues.Back-to-back navigation race condition (in 5c925f3)
A second
__unsafe_action__can fire beforeflush()resolves from the first navigation (e.g. rapid taps). Without a fix, nav-2 events would land in the queue being flushed and be attributed to view 1.prepareEndNavigation()now snapshotscallbackQueueinto_pendingFlushQueueand resets the live queue to[].flush()drains only the snapshot, so any events enqueued by a concurrent nav-2 are isolated and will be flushed in their own cycle.endNavigation()anddrain()calldrainAllQueues()which drains both, so nothing leaks on teardown/timeoutKey files
core/.../Buffer/NavigationBuffer.tsDatadogBufferdecorator implementing the buffer lifecyclecore/.../Buffer/BufferSingleton.tsonInitializationnow installsNavigationBufferwrappingPassThroughBufferas the active SDK bufferreact-navigation/.../DdRumReactNavigationTracking.tsx__unsafe_action__listener; callsprepareEndNavigation/flusharoundstartView; addsuseNavigationBufferoptioncore/src/index.tsxBufferSingletonremoved from public exports — react-navigation accesses it viagetGlobalInstanceusing the shared globalThis keyAdditional Notes
NavigationBufferis accessed fromreact-navigationviagetGlobalInstance('com.datadog.reactnative.buffer_singleton', ...)— no cross-package import. The key is the same oneBufferSingletonregisters under. A comment in both files marks this coupling.NAVIGATION_BUFFER_TIMEOUT_MS) guarantees the buffer is always drained even ifonStateChangenever fires (e.g. navigation cancelled).__unsafe_action__wiring.LOGcalls are temporary and markedTODO: remove before shipping- I will remove them after some further testing on my side, and before merging the PRReview checklist (to be filled by reviewers)