Skip to content

RUM-11386: Make Navigation3 tracking listen to lifecycle#2832

Merged
ambushwork merged 1 commit into
developfrom
yl/nav3/add-lifecycle
Aug 26, 2025
Merged

RUM-11386: Make Navigation3 tracking listen to lifecycle#2832
ambushwork merged 1 commit into
developfrom
yl/nav3/add-lifecycle

Conversation

@ambushwork

@ambushwork ambushwork commented Aug 22, 2025

Copy link
Copy Markdown
Member

What does this PR do?

This PR mainly attach the Navigation3TrackingEffect to the lifecycle of the lifecycle owner.
Also there is some improvements:

  • Move the core logic to a dedicated internal non-composable function so it can be easily unit tested.
  • Adding BackstackKeyResolver interface which can accept the custom key resolving, by default we resolve it with hashcode.

Motivation

What inspired you to submit this pull request?

Additional Notes

Anything else we should know when reviewing?

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/add-lifecycle branch 5 times, most recently from ea64539 to be891a3 Compare August 25, 2025 11:04
@codecov-commenter

codecov-commenter commented Aug 25, 2025 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 18.75000% with 39 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.93%. Comparing base (207c199) to head (92f3a43).
⚠️ Report is 1841 commits behind head on develop.

Files with missing lines Patch % Lines
.../kotlin/com/datadog/android/compose/Navigation3.kt 13.33% 36 Missing and 3 partials ⚠️
Additional details and impacted files 
@@             Coverage Diff             @@
##           develop    #2832      +/-   ##
===========================================
- Coverage    70.03%   69.93%   -0.09%     
===========================================
  Files          845      846       +1     
  Lines        31497    31530      +33     
  Branches      5283     5289       +6     
===========================================
- Hits         22056    22050       -6     
- Misses        7962     7999      +37     
- Partials      1479     1481       +2
Files with missing lines Coverage Δ
...atadog/android/compose/AcceptAllNavKeyPredicate.kt 85.71% <100.00%> (ø)
...og/android/compose/HashcodeBackStackKeyResolver.kt 100.00% <100.00%> (ø)
.../kotlin/com/datadog/android/compose/Navigation3.kt 9.30% <13.33%> (+9.30%) ⬆️

... and 34 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/add-lifecycle branch from be891a3 to a9d3543 Compare August 25, 2025 11:49
@ambushwork
ambushwork marked this pull request as ready for review August 25, 2025 12:34
@ambushwork
ambushwork requested review from a team as code owners August 25, 2025 12:34
0xnm
0xnm reviewed Aug 25, 2025
View reviewed changes
Comment thread integrations/dd-sdk-android-compose/src/main/kotlin/com/datadog/android/compose/Navigation3.kt Outdated
Comment on lines +48 to +50
backStack: List<T>,
keyPredicate: ComponentPredicate<T> = AcceptAllNavKeyPredicate(),
backstackKeyResolver: BackstackKeyResolver<T> = HashcodeBackstackKeyResolver(),

@0xnm 0xnm Aug 25, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

minor: we need to align spelling of backStack vs backstack

@ambushwork ambushwork Aug 26, 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.

To follow the Google API grammar, backStack is picked for argument name and back stack for documentation.

Comment thread ...s/dd-sdk-android-compose/src/main/kotlin/com/datadog/android/compose/BackstackKeyResolver.kt Outdated
Comment thread ...s/dd-sdk-android-compose/src/main/kotlin/com/datadog/android/compose/BackstackKeyResolver.kt Outdated
Comment thread ...s/dd-sdk-android-compose/src/main/kotlin/com/datadog/android/compose/BackstackKeyResolver.kt
Comment thread ...-android-compose/src/main/kotlin/com/datadog/android/compose/HashcodeBackstackKeyResolver.kt Outdated
@ambushwork
ambushwork force-pushed the yl/nav3/add-lifecycle branch 2 times, most recently from bdf82ee to 8f65a16 Compare August 26, 2025 06:12
@ambushwork
ambushwork requested a review from 0xnm August 26, 2025 07:13
0xnm
0xnm previously approved these changes Aug 26, 2025
View reviewed changes

@0xnm 0xnm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm, but it would be nice to have another pair of eyes

Comment thread integrations/dd-sdk-android-compose/src/main/kotlin/com/datadog/android/compose/Navigation3.kt Outdated
Comment thread ...roid-compose/src/test/kotlin/com/datadog/android/compose/HashcodeBackStackKeyResolverTest.kt Outdated
@aleksandr-gringauz

aleksandr-gringauz commented Aug 26, 2025

Copy link
Copy Markdown
Contributor

there is only this issue, otherwise LGTM

@ambushwork
ambushwork force-pushed the yl/nav3/add-lifecycle branch from 8f65a16 to f731838 Compare August 26, 2025 10:53
0xnm
0xnm previously approved these changes Aug 26, 2025
View reviewed changes
@ambushwork
ambushwork dismissed stale reviews from aleksandr-gringauz and 0xnm via e56a48b August 26, 2025 11:36
@ambushwork
ambushwork force-pushed the yl/nav3/add-lifecycle branch from f731838 to e56a48b Compare August 26, 2025 11:36
@ambushwork
ambushwork force-pushed the yl/nav3/add-lifecycle branch from e56a48b to 7ebd2e3 Compare August 26, 2025 12:58
@ambushwork
ambushwork force-pushed the yl/nav3/add-lifecycle branch from 7ebd2e3 to 92f3a43 Compare August 26, 2025 13:26
@ambushwork
ambushwork merged commit 44d37ef into develop Aug 26, 2025
25 checks passed
@ambushwork
ambushwork deleted the yl/nav3/add-lifecycle branch August 26, 2025 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aleksandr-gringauz aleksandr-gringauz aleksandr-gringauz approved these changes
@0xnm 0xnm 0xnm left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ambushwork @codecov-commenter @aleksandr-gringauz @0xnm