Skip to content

RUM-11386: Implement Navigation3 view tracking side effect#2830

Merged
ambushwork merged 1 commit into
developfrom
yl/nav3/api-implementation
Aug 22, 2025
Merged

RUM-11386: Implement Navigation3 view tracking side effect#2830
ambushwork merged 1 commit into
developfrom
yl/nav3/api-implementation

Conversation

@ambushwork

@ambushwork ambushwork commented Aug 21, 2025

Copy link
Copy Markdown
Member

What does this PR do?

Implement Navigation3 view tracking side effect

Motivation

RUM-11386

Demo

Sample app with Navigation3TrackingEffect send correct view events when navigating.

image

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • Make sure you discussed the feature or bugfix with the maintaining team in an Issue
  • Make sure each commit and the PR mention the Issue number (cf the CONTRIBUTING doc)

@ambushwork
ambushwork force-pushed the yl/nav3/api-implementation branch 3 times, most recently from d9bcd17 to bd81feb Compare August 21, 2025 10:41
@codecov-commenter

codecov-commenter commented Aug 21, 2025

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 8.57143% with 64 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.02%. Comparing base (170900c) to head (d64e40c).
⚠️ Report is 1853 commits behind head on develop.

Files with missing lines Patch % Lines
.../kotlin/com/datadog/android/compose/Navigation3.kt 0.00% 55 Missing ⚠️
.../com/datadog/android/compose/internal/Telemetry.kt 0.00% 6 Missing ⚠️
...n/kotlin/com/datadog/android/compose/Navigation.kt 0.00% 2 Missing ⚠️
...atadog/android/compose/AcceptAllNavKeyPredicate.kt 85.71% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2830      +/-   ##
===========================================
- Coverage    70.16%   70.02%   -0.13%     
===========================================
  Files          843      845       +2     
  Lines        31423    31493      +70     
  Branches      5263     5283      +20     
===========================================
+ Hits         22045    22052       +7     
- Misses        7901     7963      +62     
- Partials      1477     1478       +1     
Files with missing lines Coverage Δ
...atadog/android/compose/AcceptAllNavKeyPredicate.kt 85.71% <85.71%> (ø)
...n/kotlin/com/datadog/android/compose/Navigation.kt 0.00% <0.00%> (ø)
.../com/datadog/android/compose/internal/Telemetry.kt 0.00% <0.00%> (ø)
.../kotlin/com/datadog/android/compose/Navigation3.kt 0.00% <0.00%> (ø)

... and 35 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ambushwork
ambushwork force-pushed the yl/nav3/api-implementation branch 2 times, most recently from 76c63d1 to 767f3c4 Compare August 21, 2025 12:27
@ambushwork
ambushwork marked this pull request as ready for review August 21, 2025 12:34
@ambushwork
ambushwork requested review from a team as code owners August 21, 2025 12:34
}

@Composable
@NonRestartableComposable

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.

I know it existed before in InternalNavigationViewTrackingStrategy, but curious. Why do we need this NonRestartableComposable thing?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

If I understand correctly it tells compiler that this function is stable, never needs to be recomposed independently, it's a recomposition optimisation

@ambushwork
ambushwork force-pushed the yl/nav3/api-implementation branch from 767f3c4 to d64e40c Compare August 21, 2025 14:21
?.enrichWithConstantAttribute(ViewScopeInstrumentationType.COMPOSE)
?: emptyMap()

GlobalRumMonitor.get(sdkCore).startView(

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.

I found this stopView call when onPause in lifecycle happens. Shouldn't we do a similar thing here?

In all our "tracking strategies" (for example here) we stop the view when the app goes to background.

@ambushwork ambushwork Aug 22, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

yes, you are totally right, actually that will be my next commit, sorry for not being clear here

@aleksandr-gringauz
aleksandr-gringauz self-requested a review August 22, 2025 07:04
@ambushwork
ambushwork merged commit 207c199 into develop Aug 22, 2025
25 checks passed
@ambushwork
ambushwork deleted the yl/nav3/api-implementation branch August 22, 2025 15:38
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