Skip to content

Refactor navigation state so core is impl agnostic#3208

Closed
bidetofevil wants to merge 2 commits intohho/nav-state-navcontrollerfrom
hho/nav-state-refactor
Closed

Refactor navigation state so core is impl agnostic#3208
bidetofevil wants to merge 2 commits intohho/nav-state-navcontrollerfrom
hho/nav-state-refactor

Conversation

@bidetofevil
Copy link
Copy Markdown
Contributor

@bidetofevil bidetofevil commented Apr 10, 2026

Goal

Made the interfaces for navigation tracking implementation agnostic. That way, the NavigationStateDataSourcecode can live in its own module and code that updates states that have specific dependencies like Compose Navigation can live it its own module. All the wiring will be done via a new interface NavigationTrackingService which will have an implemenation exposed in InstrumentationArgs so the listeners and event producers can be wired up when they load (via datasources).

This offers better encapsulation and allows us to create modules with different dependencies and not have to push those onto SDK users.

Testing

Refactored existing test to ensure the rewiring still works.

/**
* Instrumentation provider that creates and registers [NavControllerTracker] with the [NavigationTrackingService].
*/
class ComposeNavigationInstrumentationProvider : InstrumentationProvider {
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.

Not sure if this is a good way to to use the data source interface because it doesn't actually provide a datasource, but it allows for the dynamic loading of the compose navigation tracking via a new module. Is there a better way to do this?

/**
* Updates the navigation state by listening to events
*/
class NavigationStateDataSource(
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 think there is potentially additional refactoring that can be done to clean this datasource up so it doesn't also fire the controller attachment events. But it's convenient right and I can do this later.

@bidetofevil bidetofevil force-pushed the hho/nav-state-refactor branch from f2b6b56 to 0de806f Compare April 10, 2026 00:43
@bidetofevil bidetofevil marked this pull request as ready for review April 10, 2026 04:51
@bidetofevil bidetofevil requested a review from a team as a code owner April 10, 2026 04:51
@bidetofevil bidetofevil requested review from fractalwrench and removed request for a team April 10, 2026 04:52
@bidetofevil bidetofevil force-pushed the hho/nav-state-refactor branch from 0de806f to 47ec7cd Compare April 10, 2026 05:01
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 10, 2026

Codecov Report

❌ Patch coverage is 89.09091% with 6 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (hho/nav-state-navcontroller@ce31e62). Learn more about missing BASE report.

Files with missing lines Patch % Lines
...ternal/navigation/NavigationTrackingServiceImpl.kt 72.72% 3 Missing ⚠️
...ntation/compose/navigation/NavControllerTracker.kt 89.28% 0 Missing and 3 partials ⚠️
Additional details and impacted files

Impacted file tree graph

@@                      Coverage Diff                       @@
##             hho/nav-state-navcontroller    #3208   +/-   ##
==============================================================
  Coverage                               ?   78.15%           
==============================================================
  Files                                  ?      583           
  Lines                                  ?    12186           
  Branches                               ?     1831           
==============================================================
  Hits                                   ?     9524           
  Misses                                 ?     1936           
  Partials                               ?      726           
Files with missing lines Coverage Δ
...oid/embracesdk/internal/InstrumentationArgsImpl.kt 86.66% <100.00%> (ø)
...k/internal/injection/EssentialServiceModuleImpl.kt 70.45% <100.00%> (ø)
...dk/internal/injection/InstrumentationModuleImpl.kt 100.00% <100.00%> (ø)
.../arch/navigation/NavigationTrackingInitListener.kt 100.00% <100.00%> (ø)
...gation/ComposeNavigationInstrumentationProvider.kt 100.00% <100.00%> (ø)
...umentation/navigation/ActivityNavigationTracker.kt 74.07% <100.00%> (ø)
...umentation/navigation/NavigationStateDataSource.kt 95.00% <100.00%> (ø)
...vigation/NavigationStateInstrumentationProvider.kt 100.00% <100.00%> (ø)
...ternal/navigation/NavigationTrackingServiceImpl.kt 72.72% <72.72%> (ø)
...ntation/compose/navigation/NavControllerTracker.kt 89.28% <89.28%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Copy Markdown
Member

@fractalwrench fractalwrench left a comment

Choose a reason for hiding this comment

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

LGTM although it'd be good to make the controller parameter a more specific type if possible

* The implementation is responsible for casting the controller to whatever interface it needs in order to do its tracking,
* as the interface is generic and agnostic to any navigation controller implementation details.
*/
fun trackNavigation(activity: Activity, controller: Any? = null)
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.

What type do we expect the controller to be? Is there any way to make it more specific than Any??

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.

For now, it's NavController. And I'd be happy to have that explicitly until there's another type, but then that means we'll have to expose this Compose Navigation dependency at the public API level.

I started out with way, but then went to this generic method. It'll be more obvious in my next PR.

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.

2 participants