Bug 1845409 - File RFC 0010 proposing handling navigation through mid…

…dleware

docs/rfcs/0010-add-state-based-navigation.md

@@ -19,28 +19,150 @@ Currently, methods of navigation throughout the app are varied. The `SessionCont
 - Calling a `NavController` directly
 - Callbacks like `showTabTray()`
 
-To move to a more consistent `Store` model, we need a way for features to fire `Action`s and have that result in navigation. This has the added benefit of decoupling our business logic from Android platform implementation details.
+To move to a more consistent Redux-like model, we need a way for features to fire `Action`s and have that result in navigation. This will help decouple our business logic from the Android platform, where a key example of this would be references to the `HomeActivity` throughout the app in order to access the `openToBrowserAndLoad` function.
 
 ## Proposal
 
-There are two cases to consider for navigation state: the currently displayed fragment and transient UI like CFRs, dialogs, etc.
+Moving forward, navigation should be initiated through middlewares that respond to `Action`s. How these middleware handle navigation side-effects can be addressed on a per-case basis, but this proposal includes some generalized advice for 3 common use cases. 
+
+### 1. Screen-based navigation
+
+For screen-based navigation between screens like the settings pages or navigation to the home screen, middlewares should make direct use of a `NavController` that is hosted by the fragment of the current screen's scope.
+
+For a hypothetical example:
+```kotlin
+
+sealed class HistoryAction {
+    object HomeButtonClicked : HistoryAction()
+	data class HistoryGroupClicked(val group: History.Group) : HistoryAction()
+}
+
+class HistoryNavigationMiddleware(private val getNavController: () -> NavController) : Middleware<HistoryState, HistoryAction> {
+	override fun invoke(
+		context: MiddlewareContext<HistoryState, HistoryAction>,
+		next: (HistoryFragmentAction) -> Unit,
+		action: HistoryFragmentAction,
+	) {
+        next(action)
+        when (action) {
+            is HomeButtonClicked -> getNavController().navigate(R.id.home_fragment)
+			is HistoryGroupClicked -> getNavController().navigate(R.id.history_metadata_fragment)
+        }
+	}
+}
+```
+
+This should translate fairly easily to the Compose world. This example intentionally ignores passing the `group` through the navigation transition. It should be fairly trivial to convert data types to navigation arguments, or consider creating Stores with a scope large enough to maintain state across these transitions.
+
+Note also the use of a lambda to retrieve the `NavController`. This should help avoid stale references when Stores outlive their parent fragment by using a `StoreProvider`.
+
+### 2. Transient effects
+
+Transient effects can be handled by callbacks provided to a middleware. To build on our previous example:
+
+```kotlin
+sealed class HistoryAction {
+    object HomeButtonClicked : HistoryAction()
+	data class HistoryGroupClicked(val group: HistoryItem.Group) : HistoryAction()
+	data class HistoryItemLongClicked(val item: HistoryItem) : HistoryAction()
+}
+
+class HistoryUiEffectMiddleware(
+    private val displayMenuForItem: (HistoryItem) -> Unit,
+) : Middleware<HistoryState, HistoryAction> {
+	override fun invoke(
+		context: MiddlewareContext<HistoryState, HistoryAction>,
+		next: (HistoryFragmentAction) -> Unit,
+		action: HistoryFragmentAction,
+	) {
+        next(action)
+        when (action) {
+            is HistoryItemLongClicked -> displayMenuForItem(action.item)
+            is HomeButtonClicked, HistoryGroupClicked -> Unit
+        }
+	}
+}
+```
+
+### 3. The special case of `openToBrowserAndLoad`
+
+Finally, we want a generally re-usable method of opening a new tab and navigating to the `BrowserFragment`. Fragment-based Stores can re-use a (theoretical) delegate to do so.
+
+```kotlin
+sealed class HistoryAction {
+    object HomeButtonClicked : HistoryAction()
+	data class HistoryGroupClicked(val group: History.Group) : HistoryAction()
+	data class HistoryItemLongClicked(val item: HistoryItem) : HistoryAction()
+	data class HistoryItemClicked(val item: History.Item) : HistoryAction()
+}
+
+class HistoryNavigationMiddleware(
+    private val browserNavigator: BrowserNavigator,
+    private val getNavController: () -> NavController,
+) : Middleware<HistoryState, HistoryAction> {
+	override fun invoke(
+		context: MiddlewareContext<HistoryState, HistoryAction>,
+		next: (HistoryFragmentAction) -> Unit,
+		action: HistoryFragmentAction,
+	) {
+        next(action)
+        when (action) {
+            is HomeButtonClicked -> navController.navigate(R.id.home_fragment)
+			is HistoryGroupClicked -> navController.navigate(R.id.history_metadata_fragment)
+			is HistoryItemClicked -> browserNavigator.openToBrowserAndLoad(action.item)
+			is HistoryItemLongClicked -> Unit
+        }
+	}
+}
+```
+
+This delegate would wrap the current behavior exposed by `HomeActivity::openToBrowserAndLoad`, looking something roughly like:
+
+```kotlin
+class BrowserNavigator(
+    private val addTabUseCase: AddNewTabUseCase,
+    private val loadTabUseCase: DefaultLoadUrlUseCase,
+    private val searchUseCases: SearchUseCases,
+    private val navController: () -> NavController,
+) {
+    // logic to navigate to browser fragment and load a tab
+}
+```
 
-For screens: add a `Screen` property to `AppStore` and react to changes by observing them in a navigation `AbstractBinding` hosted by the `HomeActivity`. This is roughly equivalent to the `Navigator` [implementation in Focus](https://searchfox.org/mozilla-mobile/rev/aa6bee71a6e0ea73f041a54ddf4d5d4e2f603429/firefox-android/focus-android/app/src/main/java/org/mozilla/focus/navigation/Navigator.kt#22). 
+## Alternatives
 
-Handling this State in AppStore should allow us to have a consistent touch point for all navigation since this Store is globally accessible. 
+### 1. Observing navigation State from a AppStore through a binding in HomeActivity.
 
-For transient state: For now, I am proposing to implement feature- or screen-specific middleware that consume actions to directly tie them to their side-effects.
+This was the previous proposal for this RFC. An example would roughly be:
 
-## Alternatives
+```kotlin
+sealed class AppAction {
+    object NavigateHome : AppAction()
+}
+
+data class AppState(
+    val currentScreen: Screen
+)
+
+fun appReducer(state: AppState, action: AppAction): AppState = when (action) {
+    is NavigateHome -> state.copy(currentScreen = Screen.Home)
+}
+
+// in HomeActivity
+private val navigationObserver by lazy {
+	object : AbstractBinding<AppState>(components.appStore) {
+		override suspend fun onState(flow: Flow<AppState>) = flow
+			.distinctUntilChangedBy { it.screen }
+			.collectLatest { /* handleNavigation */ }
+	}
+}
+```
 
-### For screens:
-One big alternative consideration is: do we want to track nav State in the Store at all? It is potentially error-prone or repetitious of other code (nav-graph and the code that actually invokes the NavController). The main alternative would be to handle navigation in middleware as a reaction to `Action`s, but options for this come with there own set of challenges:
+However, this implies some several issues:
+1. We end up replicating the state of a `NavController` manually in a our custom State, risking out-of-sync issues.
+2. We lose specificity of Actions by generalizing them globally. For example, instead of a `ToolbarAction.HomeClicked`, it would encourage re-use of a single `AppAction.NavigateHome`. Though seemingly convenient at first, it implies downstream problems for things like telemetry. To know where the navigation to home originated from, we would need to include additional properties (like direction) in the `Action`. Any future changes to the behavior of these Actions would need to be generalized for the whole app. 
 
-1. Global navigation middleware attached to the AppStore. This is made more difficult because we do not have access to an `Activity` context (to retrieve a `NavController` from) when instantiating the `AppStore` in `Component`s. We could create a mutable getter/setter for the Middleware's NavController, but this carries risks for things like race conditions, public mutability, and null accesses if the Middleware held a dangling reference to a null activity after destructive lifecycle events.
-2. Individual navigation middleware attached to each Fragment's Store. This carries risk of repetition and boilerplate.
+### 2. Global navigation middleware attached to the AppStore. 
 
-### For transient UI state:
-1. Focus handles some cases of this similarly to screens. There are examples in of CFRs in [Focus' AppState](https://searchfox.org/mozilla-mobile/rev/ebe8346d6074c72af319e3f47d9dec49de381533/firefox-android/focus-android/app/src/main/java/org/mozilla/focus/state/AppState.kt#33) for example. This is potentially more in line with the proposal to add a `Screen` property, but it carries its own risks:
-	1. State must be consumed correctly, or configuration changes can cause effects like dialogs or CFRs to erroneously re-display.
-	2. It suffers from the main drawback of the `Screen` proposal, in that we introduce additional code to track what should be treated as a side-effect (and therefore handled by middleware).
+This carries risk of the 2 issue listed above, and runs into immediate technical constraints. When the `AppStore` is constructed in `Core`, we do not have reference to an `Activity` and cannot retrieve a `NavController`. This could be mitigated by a mutable property or lazy getter that is set as Fragments or the Activity come into and out of scope. The current proposal will localize navigation transitions to feature areas which should keep them isolated in scope.