Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
7 changes: 7 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,13 @@

## Unreleased

### Features

- Multi-instance `<TimeToInitialDisplay>` / `<TimeToFullDisplay>` coordination ([#6090](https://github.com/getsentry/sentry-react-native/pull/6090)
- When a screen has multiple async data sources, you can now mount one `<TimeToFullDisplay>` per source โ€” the TTID/TTFD will get recorded
only when all the sources report `ready`.
- The new `ready` prop is declarative and replaces the previously imperative `record` prop, which is now marked as deprecated.

### Fixes

- Fix the issue with uploading iOS Debug Symbols in EAS Build when using pnpm ([#6076](https://github.com/getsentry/sentry-react-native/issues/6076))
Expand Down
139 changes: 139 additions & 0 deletions packages/core/src/js/tracing/timeToDisplayCoordinator.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,139 @@
/**
* Coordinator for multi-instance `<TimeToInitialDisplay>` / `<TimeToFullDisplay>`
* components on a single screen (active span).
*/

type Checkpoint = { ready: boolean };
type Listener = () => void;

interface SpanRegistry {
checkpoints: Map<string, Checkpoint>;
listeners: Set<Listener>;
}

const TTID = 'ttid';
const TTFD = 'ttfd';

export type DisplayKind = typeof TTID | typeof TTFD;

const registries: Record<DisplayKind, Map<string, SpanRegistry>> = {
ttid: new Map(),
ttfd: new Map(),
};

function getOrCreate(kind: DisplayKind, parentSpanId: string): SpanRegistry {
const map = registries[kind];
let entry = map.get(parentSpanId);
if (!entry) {
entry = {
checkpoints: new Map(),
listeners: new Set()
};
map.set(parentSpanId, entry);
}
return entry;
}

function performCleanup(kind: DisplayKind, parentSpanId: string, entry: SpanRegistry): void {
if (entry.checkpoints.size === 0 && entry.listeners.size === 0) {
registries[kind].delete(parentSpanId);
}
}

Check warning on line 41 in packages/core/src/js/tracing/timeToDisplayCoordinator.ts

View check run for this annotation

@sentry/warden / warden: code-review

Coordinator registries are never evicted on span end, risking unbounded growth

`registries.ttid` / `registries.ttfd` are keyed by `parentSpanId` and only cleaned up when both `checkpoints` and `listeners` reach size 0 via `performCleanup`. There is no hook tied to the active span ending, so if a `<TimeToInitialDisplay>` / `<TimeToFullDisplay>` component is still mounted (or its unregister never runs because of an unmount race or an error path) when the screen/span finishes, the entry stays in the module-level Map forever. Across many navigations this becomes a slow memory leak. Note that the sibling `spanFrameDataMap` in `timetodisplay.tsx` explicitly added a `FRAME_DATA_CLEANUP_TIMEOUT_MS` safety net for exactly this scenario; the coordinator has no equivalent safeguard.
Comment thread
sentry-warden[bot] marked this conversation as resolved.
Comment thread
sentry-warden[bot] marked this conversation as resolved.

/**
* Register a checkpoint under (kind, parentSpanId). Returns an unregister fn.
*/
export function registerCheckpoint(
kind: DisplayKind,
parentSpanId: string,
checkpointId: string,
ready: boolean,
): () => void {
const entry = getOrCreate(kind, parentSpanId);
entry.checkpoints.set(checkpointId, { ready });
notify(entry);

return () => {
const e = registries[kind].get(parentSpanId);
if (!e) {
return;
}
if (e.checkpoints.delete(checkpointId)) {
notify(e);
}
performCleanup(kind, parentSpanId, e);
};
Comment thread
cursor[bot] marked this conversation as resolved.
}

/**
* Update an existing checkpoint's ready state.
*/
export function updateCheckpoint(
kind: DisplayKind,
parentSpanId: string,
checkpointId: string,
ready: boolean,
): void {
const entry = registries[kind].get(parentSpanId);
const cp = entry?.checkpoints.get(checkpointId);
if (!entry || !cp || cp.ready === ready) {
return;
}
cp.ready = ready;
notify(entry);
}

/**
* True if at least one checkpoint is registered AND all checkpoints are ready.
*/
export function isAllReady(kind: DisplayKind, parentSpanId: string): boolean {
const entry = registries[kind].get(parentSpanId);
if (!entry || entry.checkpoints.size === 0) {
return false;
}
for (const cp of entry.checkpoints.values()) {
if (!cp.ready) {
return false;
}
}
return true;
}

/**
* Returns true if there is at least one registered checkpoint on this span.
*/
export function hasAnyCheckpoints(kind: DisplayKind, parentSpanId: string): boolean {
const entry = registries[kind].get(parentSpanId);
return !!entry && entry.checkpoints.size > 0;
}

/**
* Subscribe to any checkpoint state change for a given span. The listener is
* called synchronously after each register/update/unregister event.
*/
export function subscribe(kind: DisplayKind, parentSpanId: string, listener: Listener): () => void {
const entry = getOrCreate(kind, parentSpanId);
entry.listeners.add(listener);
return () => {
const e = registries[kind].get(parentSpanId);
if (!e) {
return;
}
e.listeners.delete(listener);
performCleanup(kind, parentSpanId, e);
};
}

function notify(entry: SpanRegistry): void {
for (const listener of entry.listeners) {
listener();
}
}

/**
* Test-only. Clears all coordinator state.
*/
export function _resetTimeToDisplayCoordinator(): void {
registries.ttid.clear();
registries.ttfd.clear();
}
107 changes: 100 additions & 7 deletions packages/core/src/js/tracing/timetodisplay.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,14 @@
startInactiveSpan,
} from '@sentry/core';
import * as React from 'react';
import { useState } from 'react';
import { useEffect, useReducer, useRef, useState } from 'react';

import type { NativeFramesResponse } from '../NativeRNSentry';

import { NATIVE } from '../wrapper';
import { SPAN_ORIGIN_AUTO_UI_TIME_TO_DISPLAY, SPAN_ORIGIN_MANUAL_UI_TIME_TO_DISPLAY } from './origin';
import type { DisplayKind } from './timeToDisplayCoordinator';
import { isAllReady, registerCheckpoint, subscribe, updateCheckpoint } from './timeToDisplayCoordinator';
import { getRNSentryOnDrawReporter } from './timetodisplaynative';
import { setSpanDurationAsMeasurement, setSpanDurationAsMeasurementOnSpan } from './utils';

Expand Down Expand Up @@ -59,15 +61,31 @@

export type TimeToDisplayProps = {
Comment thread
alwx marked this conversation as resolved.
children?: React.ReactNode;
/**
* @deprecated Use `ready` instead. `record` and `ready` are equivalent;
* `record` will be removed in the next major version.
*/
record?: boolean;
/**
* Marks this checkpoint as ready. The display is recorded only when every
* `<TimeToFullDisplay>` / `<TimeToInitialDisplay>` mounted under the
* currently active span reports `ready === true`.
*
* <TimeToFullDisplay ready={feedReady} />
* <TimeToFullDisplay ready={sidebarReady} />
*/
ready?: boolean;

Check warning on line 77 in packages/core/src/js/tracing/timetodisplay.tsx

View check run for this annotation

@sentry/warden / warden: find-bugs

`ready` prop bypasses focus-effect gating in createTimeToDisplay wrappers

The new `ready` prop added at line 77 takes precedence over `record` in `useCoordinatedDisplay` (line 153). However, `createTimeToDisplay` (line 511-536, outside this hunk) only overrides `record={focused && props.record}` and forwards `ready` unchanged via `{...props}`. As a result, components built via `createTimeToFullDisplay`/`createTimeToInitialDisplay` with `useFocusEffect` no longer gate display recording on screen focus when callers pass `ready` โ€” the display can be recorded on a non-focused screen, producing incorrect TTID/TTFD measurements during navigation.
Comment thread
sentry-warden[bot] marked this conversation as resolved.
};

/**
* Component to measure time to initial display.
*
* The initial display is recorded when the component prop `record` is true.
* Single instance:
* <TimeToInitialDisplay ready={isLoaded} />
*
* <TimeToInitialDisplay record />
* Multiple instances coordinating on one screen:
* <TimeToInitialDisplay ready={headerLoaded} />
* <TimeToInitialDisplay ready={contentLoaded} />
*/
export function TimeToInitialDisplay(props: TimeToDisplayProps): React.ReactElement {
const activeSpan = getActiveSpan();
Expand All @@ -76,8 +94,10 @@
}

const parentSpanId = activeSpan && spanToJSON(activeSpan).span_id;
const initialDisplay = useCoordinatedDisplay('ttid', parentSpanId, props);

return (
<TimeToDisplay initialDisplay={props.record} parentSpanId={parentSpanId}>
<TimeToDisplay initialDisplay={initialDisplay} parentSpanId={parentSpanId}>
{props.children}
</TimeToDisplay>
);
Expand All @@ -86,20 +106,93 @@
/**
* Component to measure time to full display.
*
* The initial display is recorded when the component prop `record` is true.
* Single instance:
* <TimeToFullDisplay ready={isLoaded} />
*
* <TimeToInitialDisplay record />
* Multiple instances coordinating on one screen:
* <TimeToFullDisplay ready={feedReady} />
* <TimeToFullDisplay ready={sidebarReady} />
*/
export function TimeToFullDisplay(props: TimeToDisplayProps): React.ReactElement {
const activeSpan = getActiveSpan();
const parentSpanId = activeSpan && spanToJSON(activeSpan).span_id;
const fullDisplay = useCoordinatedDisplay('ttfd', parentSpanId, props);

return (
<TimeToDisplay fullDisplay={props.record} parentSpanId={parentSpanId}>
<TimeToDisplay fullDisplay={fullDisplay} parentSpanId={parentSpanId}>
{props.children}
</TimeToDisplay>
);
}
Comment thread
sentry-warden[bot] marked this conversation as resolved.

/**
* Every `<TimeToInitialDisplay>` / `<TimeToFullDisplay>` instance registers as
* a checkpoint under the active span. The aggregate is ready if every
* checkpoint reports ready.
*/
/**
* Module-local counter used to mint stable, unique checkpoint ids per
* component instance without requiring React 18's `useId`.
*/
let nextCheckpointId = 0;

function useCoordinatedDisplay(
kind: DisplayKind,
parentSpanId: string | undefined,
props: TimeToDisplayProps,
): boolean {
// Stable per-instance id. `useRef` is available since React 16.8.
const checkpointIdRef = useRef<string | null>(null);
if (checkpointIdRef.current === null) {
checkpointIdRef.current = `cp-${nextCheckpointId++}`;
}
const checkpointId = checkpointIdRef.current;
const [, force] = useReducer((x: number) => x + 1, 0);

// `ready` takes precedence when both are provided.
const localReady = props.ready !== undefined ? !!props.ready : !!props.record;

if (__DEV__) {
if (props.ready !== undefined && props.record !== undefined) {
debug.warn('[TimeToDisplay] Both `ready` and `record` were provided โ€” ignoring `record`.');
}
if (props.record !== undefined) {
debug.warn('[TimeToDisplay] The `record` prop is deprecated. Use `ready` instead.');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's create an issue on v9 to remove the deprecation

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}
}
Comment thread
sentry-warden[bot] marked this conversation as resolved.
Outdated

// Subscribe FIRST so this component receives its own registration notify
// (and any peer notifications) on mount.
useEffect(() => {
if (!parentSpanId) {
return undefined;
}
return subscribe(kind, parentSpanId, force);
}, [kind, parentSpanId]);

// Register on mount / when the active span changes; unregister on unmount.
useEffect(() => {
if (!parentSpanId) {
return undefined;
}
return registerCheckpoint(kind, parentSpanId, checkpointId, localReady);
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [kind, parentSpanId, checkpointId]);

// Propagate ready transitions to the registry.
useEffect(() => {
if (!parentSpanId) {
return;
}
updateCheckpoint(kind, parentSpanId, checkpointId, localReady);
}, [kind, parentSpanId, checkpointId, localReady]);

if (!parentSpanId) {
return false;
}
return isAllReady(kind, parentSpanId);

Check warning on line 193 in packages/core/src/js/tracing/timetodisplay.tsx

View check run for this annotation

@sentry/warden / warden: code-review

isAllReady on first render can return true before this instance registers, causing premature TTID/TTFD recording

useCoordinatedDisplay calls isAllReady synchronously during render, but registerCheckpoint runs in useEffect after commit. On the very first render of a newly mounted instance, its checkpoint is not yet in the registry. If a peer instance is already registered and ready under the same parentSpanId, isAllReady returns true and the returned boolean flows into TimeToDisplay as initialDisplay/fullDisplay โ€” potentially recording the display before this new checkpoint has had a chance to report ready=false. This contradicts the documented contract that the display is recorded only when every mounted checkpoint reports ready.
Comment thread
sentry-warden[bot] marked this conversation as resolved.
Outdated
Comment thread
sentry-warden[bot] marked this conversation as resolved.
Comment thread
cursor[bot] marked this conversation as resolved.
}
Comment thread
sentry-warden[bot] marked this conversation as resolved.

Comment thread
cursor[bot] marked this conversation as resolved.
function TimeToDisplay(props: {
children?: React.ReactNode;
initialDisplay?: boolean;
Expand Down
Loading
Loading