Skip to content

Commit 273758a

Browse files
committed
Fixes
1 parent 9ef82b8 commit 273758a

6 files changed

Lines changed: 334 additions & 15 deletions

File tree

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
```
2020
- The existing `record` prop is **unchanged** and continues to behave exactly as before — instances using `record` are independent and do not gate or get gated by `ready` peers. Existing apps require no migration.
2121
- `record` is now deprecated in favor of `ready`. Migrating to `ready` is a one-line rename that opts the instance into multi-instance coordination. `record` will be removed in the next major version.
22+
- Mounting a not-yet-ready peer on the next render commit (typical `useEffect` → `setState` → child mount waves) cancels any pending fire from an earlier-ready peer, preventing premature TTFD recording. Up-flips of the aggregate are deferred by one macrotask to absorb same-task peer mounts; down-flips are immediate.
2223

2324
### Fixes
2425

packages/core/src/js/tracing/integrations/timeToDisplayIntegration.ts

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import { debug } from '@sentry/core';
44

55
import { NATIVE } from '../../wrapper';
66
import { UI_LOAD_FULL_DISPLAY, UI_LOAD_INITIAL_DISPLAY } from '../ops';
7+
import { clearSpan as clearTimeToDisplayCoordinatorSpan } from '../timeToDisplayCoordinator';
78
import { SPAN_ORIGIN_AUTO_UI_TIME_TO_DISPLAY, SPAN_ORIGIN_MANUAL_UI_TIME_TO_DISPLAY } from '../origin';
89
import { getReactNavigationIntegration } from '../reactnavigation';
910
import { SEMANTIC_ATTRIBUTE_ROUTE_HAS_BEEN_SEEN } from '../semanticAttributes';
@@ -86,6 +87,12 @@ export const timeToDisplayIntegration = (): Integration => {
8687
event.timestamp = newTransactionEndTimestampSeconds;
8788
}
8889

90+
// Drop the per-span coordinator state now that we've read the native
91+
// ttid/ttfd values. Prevents the module-level registries from
92+
// accumulating entries for screens that outlive their span (keep-alive,
93+
// idle-timeout discarded transactions, etc.).
94+
clearTimeToDisplayCoordinatorSpan(rootSpanId);
95+
8996
return event;
9097
},
9198
};

packages/core/src/js/tracing/timeToDisplayCoordinator.ts

Lines changed: 116 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,27 @@
11
/**
22
* Coordinator for multi-instance `<TimeToInitialDisplay>` / `<TimeToFullDisplay>`
33
* components on a single screen (active span).
4+
*
5+
* The aggregate "ready" state exposed via `isAllReady` is *deferred* on its
6+
* way up: when the raw aggregate flips false→true, we schedule the public
7+
* value to flip on the next macrotask. Down-flips (true→false) are
8+
* immediate and cancel any pending up-flip.
9+
*
10+
* Why: in React 18, a typical "parent renders → parent useEffect setState
11+
* → child mounts on next commit" wave executes synchronously inside one
12+
* event-loop task. A `setTimeout(0)` reliably runs after that whole wave,
13+
* so cross-commit-but-same-task peer registrations are absorbed before the
14+
* coordinator declares itself ready. Without the defer, a header that
15+
* registers and is alone-and-ready would emit `fullDisplay=true`
16+
* immediately, the native reporter would fire on the next draw, and a
17+
* sibling sidebar mounting on the next commit could only un-ready the
18+
* aggregate after the (now stuck) native timestamp has already been
19+
* recorded.
20+
*
21+
* The defer does NOT cover arbitrary-async deferred mounting (e.g. mount a
22+
* checkpoint after a fetch resolves). That class of usage is documented
23+
* against — the recommended pattern is to mount all checkpoints at screen
24+
* mount with `ready=false` and flip them as data arrives.
425
*/
526

627
type Checkpoint = { ready: boolean };
@@ -10,12 +31,20 @@ interface SpanRegistry {
1031
checkpoints: Map<string, Checkpoint>;
1132
listeners: Set<Listener>;
1233
/**
13-
* Last-observed aggregate ready state. Used to avoid waking subscribers when
14-
* a checkpoint change does not flip the aggregate — the dominant lifecycle
15-
* pattern is "all checkpoints register as not-ready, then flip to ready over
16-
* time", and only the final flip needs to notify.
34+
* Stable, deferred view of the aggregate exposed via `isAllReady`. Lags
35+
* raw `computeAggregate` on up-flips by `READY_DEFER_MS`, immediate on
36+
* down-flips. Used to avoid waking subscribers when a checkpoint change
37+
* does not flip the aggregate — the dominant lifecycle pattern is "all
38+
* checkpoints register as not-ready, then flip to ready over time", and
39+
* only the final flip needs to notify.
1740
*/
1841
aggregateReady: boolean;
42+
/**
43+
* Pending up-flip timer. When non-null, an up-flip is scheduled but has
44+
* not yet been applied to `aggregateReady`. Cleared either when the timer
45+
* fires or when an intervening change cancels the pending up-flip.
46+
*/
47+
pendingUpFlip: ReturnType<typeof setTimeout> | null;
1948
/**
2049
* Checkpoint ids whose components have unmounted but are kept in the
2150
* registry to prevent a premature aggregate flip (sole-blocker safeguard).
@@ -25,6 +54,12 @@ interface SpanRegistry {
2554
sticky: Set<string>;
2655
}
2756

57+
/**
58+
* Defer applied to up-flips. Zero macrotask is enough to absorb a same-task
59+
* cascade of useEffect-driven child mounts in React 18.
60+
*/
61+
const READY_DEFER_MS = 0;
62+
2863
const TTID = 'ttid';
2964
const TTFD = 'ttfd';
3065

@@ -43,13 +78,21 @@ function getOrCreate(kind: DisplayKind, parentSpanId: string): SpanRegistry {
4378
checkpoints: new Map(),
4479
listeners: new Set(),
4580
aggregateReady: false,
81+
pendingUpFlip: null,
4682
sticky: new Set(),
4783
};
4884
map.set(parentSpanId, entry);
4985
}
5086
return entry;
5187
}
5288

89+
function cancelPendingUpFlip(entry: SpanRegistry): void {
90+
if (entry.pendingUpFlip !== null) {
91+
clearTimeout(entry.pendingUpFlip);
92+
entry.pendingUpFlip = null;
93+
}
94+
}
95+
5396
function computeAggregate(entry: SpanRegistry): boolean {
5497
if (entry.checkpoints.size === 0) {
5598
return false;
@@ -63,17 +106,47 @@ function computeAggregate(entry: SpanRegistry): boolean {
63106
}
64107

65108
/**
66-
* Recompute the aggregate; if it flipped, update the cached value and notify.
67-
* No-op when the aggregate is unchanged — this is what avoids the O(N²)
68-
* notify-storm when many checkpoints register/update without crossing the
69-
* aggregate boundary.
109+
* Recompute the raw aggregate and reconcile it with the cached
110+
* `aggregateReady`. Up-flips are deferred to absorb same-task peer mounts;
111+
* down-flips are immediate and cancel any pending up-flip.
112+
*
113+
* Transition matrix (raw, stable) → action:
114+
* (false, false): no-op; cancel pending up-flip if any (it became stale).
115+
* (true, true): no-op; cancel pending up-flip if any.
116+
* (false, true): immediate down-flip + notify; cancel pending up-flip.
117+
* (true, false): schedule up-flip if not already pending.
70118
*/
71119
function reevaluate(entry: SpanRegistry): void {
72-
const next = computeAggregate(entry);
73-
if (next === entry.aggregateReady) {
120+
const raw = computeAggregate(entry);
121+
122+
if (raw === entry.aggregateReady) {
123+
cancelPendingUpFlip(entry);
74124
return;
75125
}
76-
entry.aggregateReady = next;
126+
127+
if (!raw) {
128+
cancelPendingUpFlip(entry);
129+
entry.aggregateReady = false;
130+
notifyListeners(entry);
131+
return;
132+
}
133+
134+
// raw=true, stable=false: schedule deferred up-flip.
135+
if (entry.pendingUpFlip !== null) {
136+
return;
137+
}
138+
entry.pendingUpFlip = setTimeout(() => {
139+
entry.pendingUpFlip = null;
140+
// Re-check on fire — a peer may have un-readied between schedule and now.
141+
if (!computeAggregate(entry) || entry.aggregateReady) {
142+
return;
143+
}
144+
entry.aggregateReady = true;
145+
notifyListeners(entry);
146+
}, READY_DEFER_MS);
147+
}
148+
149+
function notifyListeners(entry: SpanRegistry): void {
77150
for (const listener of entry.listeners) {
78151
listener();
79152
}
@@ -92,6 +165,7 @@ function reevaluate(entry: SpanRegistry): void {
92165
function performCleanup(kind: DisplayKind, parentSpanId: string, entry: SpanRegistry): void {
93166
const liveCheckpoints = entry.checkpoints.size - entry.sticky.size;
94167
if (liveCheckpoints === 0 && entry.listeners.size === 0) {
168+
cancelPendingUpFlip(entry);
95169
registries[kind].delete(parentSpanId);
96170
}
97171
}
@@ -217,10 +291,39 @@ export function subscribe(kind: DisplayKind, parentSpanId: string, listener: Lis
217291
};
218292
}
219293

294+
/**
295+
* Drop coordinator state for `parentSpanId` across both kinds.
296+
*
297+
* Called by the time-to-display integration once a transaction has been
298+
* processed, since the per-span coordinator state is no longer relevant
299+
* after the native draw timestamps have been read. Without this hook,
300+
* entries for screens that stay mounted past the end of their span
301+
* (React Navigation keep-alive, idle-timeout discarded transactions,
302+
* etc.) would accumulate in the module-level registries.
303+
*
304+
* Components that are still subscribed when their span is cleared remain
305+
* functional: their next interaction recreates the entry under the same
306+
* (now stale) parentSpanId. Since the integration has already read the
307+
* native ttid/ttfd values for that span, any subsequent fires are inert.
308+
*/
309+
export function clearSpan(parentSpanId: string): void {
310+
for (const kind of [TTID, TTFD] as const) {
311+
const entry = registries[kind].get(parentSpanId);
312+
if (entry) {
313+
cancelPendingUpFlip(entry);
314+
registries[kind].delete(parentSpanId);
315+
}
316+
}
317+
}
318+
220319
/**
221320
* Test-only. Clears all coordinator state.
222321
*/
223322
export function _resetTimeToDisplayCoordinator(): void {
224-
registries.ttid.clear();
225-
registries.ttfd.clear();
323+
for (const kind of [TTID, TTFD] as const) {
324+
for (const entry of registries[kind].values()) {
325+
cancelPendingUpFlip(entry);
326+
}
327+
registries[kind].clear();
328+
}
226329
}

packages/core/src/js/tracing/timetodisplay.tsx

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -189,14 +189,26 @@ function useCoordinatedDisplay(
189189
return subscribe(kind, parentSpanId, force);
190190
}, [kind, parentSpanId, useRegistry]);
191191

192+
// Tracks whether this component's checkpoint is currently registered with
193+
// the coordinator. Used to detect the pre-register render so we don't
194+
// inherit a peer's ready=true verdict before our own checkpoint exists in
195+
// the aggregate. Reset on unregister so re-mount under a new active span
196+
// re-clamps correctly.
197+
const registeredRef = useRef(false);
198+
192199
// Register on mount / when the active span changes; unregister on unmount.
193200
// Legacy-mode components do not register — they are independent and don't
194201
// gate or get gated by peers.
195202
useEffect(() => {
196203
if (!parentSpanId || !useRegistry) {
197204
return undefined;
198205
}
199-
return registerCheckpoint(kind, parentSpanId, checkpointId, localReady);
206+
const unregister = registerCheckpoint(kind, parentSpanId, checkpointId, localReady);
207+
registeredRef.current = true;
208+
return () => {
209+
registeredRef.current = false;
210+
unregister();
211+
};
200212
// eslint-disable-next-line react-hooks/exhaustive-deps
201213
}, [kind, parentSpanId, useRegistry, checkpointId]);
202214

@@ -217,7 +229,15 @@ function useCoordinatedDisplay(
217229
if (!useRegistry) {
218230
return localReady;
219231
}
220-
// Registry: gated on the per-span aggregate.
232+
// Registry, pre-register render: our checkpoint is not yet in the
233+
// registry, so `isAllReady` reflects only peer state. Combine it with our
234+
// own `localReady` so we never inherit a peer's true verdict before our
235+
// own checkpoint has had a chance to report.
236+
if (!registeredRef.current) {
237+
return localReady && isAllReady(kind, parentSpanId);
238+
}
239+
// Registry, post-register: gated on the per-span aggregate, which now
240+
// includes our checkpoint.
221241
return isAllReady(kind, parentSpanId);
222242
}
223243

0 commit comments

Comments
 (0)