Skip to content

fix(tracing): Guard getNewScreenTimeToDisplay behind enableTimeToInitialDisplay#5849

Merged
antonis merged 6 commits intomainfrom
antonis/guard-native-calls
Mar 20, 2026
Merged

fix(tracing): Guard getNewScreenTimeToDisplay behind enableTimeToInitialDisplay#5849
antonis merged 6 commits intomainfrom
antonis/guard-native-calls

Conversation

@antonis
Copy link
Copy Markdown
Contributor

@antonis antonis commented Mar 19, 2026

📢 Type of change

  • Bugfix

📜 Description

NATIVE.getNewScreenTimeToDisplay() was called on every navigation state change regardless of the enableTimeToInitialDisplay option. This native bridge call posts to the main thread and registers a Choreographer callback (Android) or CADisplayLink (iOS) to capture the next frame timestamp.

The dispatch-time native calls (setActiveSpanId, navigationProcessingSpan) were already correctly guarded behind the enableTimeToInitialDisplay flag. The state-change-time call was the only one missing the guard.

💡 Motivation and Context

A customer reported significant performance overhead on low-end devices (Fire TV, Chromecast) even after setting enableTimeToInitialDisplay: false. This unguarded native bridge call was still executing on every navigation, competing with the main thread on resource-constrained devices.

Related issue: #5665

💚 How did you test it?

Added a new test in reactnavigation.ttid.test.tsx:

  • should not call getNewScreenTimeToDisplay when ttid is disabled — verifies the native bridge call is not made when enableTimeToInitialDisplay is false

All 94 reactnavigation tests pass (93 existing + 1 new).

📝 Checklist

  • I added tests to verify changes
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled
  • I updated the docs if needed.
  • All tests passing
  • No breaking changes

🔮 Next steps

Part of a broader investigation into reactNavigationIntegration performance on low-end devices (#5665).

…ialDisplay

NATIVE.getNewScreenTimeToDisplay() was called on every navigation state
change regardless of whether enableTimeToInitialDisplay was enabled.
This native bridge call posts to the main thread and registers a
Choreographer callback (Android) or CADisplayLink (iOS) to capture the
next frame timestamp.

On low-end devices (Fire TV, Chromecast) this is measurable overhead
that the user could not avoid even after setting
enableTimeToInitialDisplay: false.

The dispatch-time native calls (setActiveSpanId, navigationProcessingSpan)
were already correctly guarded behind the flag. This aligns the
state-change-time call with the same guard.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 19, 2026

Semver Impact of This PR

None (no version bump detected)

📋 Changelog Preview

This is how your changes will appear in the changelog.
Entries from this PR are highlighted with a left border (blockquote style).


  • fix(tracing): Guard getNewScreenTimeToDisplay behind enableTimeToInitialDisplay by antonis in #5849
  • chore(docs): Add changelog entry for duplicated breadcrumbs fix by antonis in #5851
  • fix(tracing): Unsubscribe spanEnd listeners after they fire to prevent accumulation by antonis in #5840
  • fix(android): Properly remove duplicated breadcrumbs by vovkasm in #5841
  • fix(tracing): Skip native frames and stall tracking for unsampled spans by antonis in #5842

🤖 This preview updates automatically when you update the PR.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 19, 2026

Fails
🚫 Pull request is not ready for merge, please add the "ready-to-merge" label to the pull request

Generated by 🚫 dangerJS against 93ca90f

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Comment thread CHANGELOG.md
@antonis antonis marked this pull request as ready for review March 19, 2026 15:48
}

addTimeToInitialDisplayFallback(latestNavigationSpan.spanContext().spanId, NATIVE.getNewScreenTimeToDisplay());
if (enableTimeToInitialDisplay) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Q: should we mention the performance overhead when enabling this on changelog/docs?

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.

I've added a note in the parameter documentation dd81064
I'll try to measure the actual performance overhead and update the doc accordingly

Copy link
Copy Markdown
Collaborator

@lucas-zimerman lucas-zimerman left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Copy Markdown
Contributor

@alwx alwx left a comment

Choose a reason for hiding this comment

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

Looks good!

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
antonis

This comment was marked as duplicate.

@antonis antonis enabled auto-merge (squash) March 20, 2026 09:17
@antonis antonis merged commit 1cbbe64 into main Mar 20, 2026
32 of 45 checks passed
@antonis antonis deleted the antonis/guard-native-calls branch March 20, 2026 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants