Fix cross-graph navigation crash by hoisting events to root controller

[Kartikey15dem]

Fixes - Jira-#MIFOSAC-638

Description

This PR fixes a runtime crash occurring when attempting to navigate from the Client/Loan screen (inside the authenticated tabs) to the Data Table screen.

The Problem (Root Cause)

The app uses a nested navigation structure:

1. Root NavHost: Manages global screens like AuthenticatedNavbar and DataTable.
2. Nested NavHost: Manages the tabs inside AuthenticatedNavbar (including ClientNavGraph).

The crash (java.lang.IllegalArgumentException) occurred because we were triggering navigation to DataTableRoute using the nested NavController. Since DataTableRoute is only registered in the Root NavHost, the nested controller could not resolve the destination and crashed.

Behavior Changes

Before	After
loanbef.webm	loanaft.webm

The Solution

I applied the Event Hoisting pattern to bridge the two navigation graphs:

1. Updated clientNavGraph to expose a lambda callback (onMoreInfoClicked) instead of accepting a NavController to perform the navigation directly.
2. Hoisted this event up through the UI layer (AuthenticatedNavbarNavigationScreen).
3. Connected the callback in authenticatedNavbarGraph (the parent), where the Root NavController is available.

Changes

• Modified authenticatedNavbarGraph to accept onMoreInfoClicked.
• Passed navController::navigateToDataTable into the graph at the root level.
• Ensured navigation commands for global screens are now executed by the parent controller, not the child.

Verification

• Verified that clicking "More Info" on a Client successfully opens the Data Table screen without crashing.
• Verified that back navigation from Data Table works correctly and preserves the tab state.

Summary by CodeRabbit

• New Features

  • Added a "More info" action that lets the app open detailed data views from client and document screens.

• Chores

  • Propagated the new action through the authenticated and client navigation flows so "More info" works consistently across the app.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

📝 Walkthrough

Walkthrough

Added a new callback parameter onMoreInfoClicked: (String, Int) -> Unit and threaded it from the authenticated navbar entry point through navbar routing into the client navigation graph, enabling a "more info" action to be handled with a String+Int context.

Changes

Cohort / File(s)	Summary
Authenticated Navbar UI & Content cmp-navigation/src/commonMain/kotlin/cmp/navigation/authenticated/AuthenticatedNavbarNavigationScreen.kt	Added public onMoreInfoClicked: (String, Int) -> Unit parameter to the screen and forwarded it into content and downstream navigation usage.
Authenticated Navbar Route cmp-navigation/src/commonMain/kotlin/cmp/navigation/authenticated/AuthenticatedNavbarRoute.kt	Updated authenticatedNavbarGraph signature to accept onMoreInfoClicked and forwarded it into the navbar screen invocation.
Authenticated Navigation Wiring cmp-navigation/src/commonMain/kotlin/cmp/navigation/authenticated/AuthenticatedNavigation.kt	Imported navigateToDataTable and wired it as the onMoreInfoClicked handler passed into authenticatedNavbarGraph.
Client Navigation Graph feature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.kt	Added onMoreInfoClicked: (String, Int) -> Unit parameter to clientNavGraph and passed it into loanDestination (replacing the previous direct navController::navigateToDataTable binding).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

• feat : collateral data screen added #2546 — Modifies client navigation mappings and destinations; likely related to clientNavGraph wiring changes.

Poem

Hopping through stacks with a twitch of my ear, 🐰
A tiny callback scurries, now routes persevere,
String and Int held snug in a paw so light,
Paths connect and whisper, guiding the flight,
I nibble at code and celebrate—more info takes flight! 🎉

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: fixing a cross-graph navigation crash by hoisting events to the root controller, which aligns with the code modifications that propagate the onMoreInfoClicked callback up the navigation hierarchy.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

Tip

🧪 Unit Test Generation v2 is now available!

We have significantly improved our unit test generation capabilities.

To enable: Add this to your .coderabbit.yaml configuration:

reviews:
  finishing_touches:
    unit_tests:
      enabled: true

Try it out by using the @coderabbitai generate unit tests command on your code files or under ✨ Finishing Touches on the walkthrough!

Have feedback? Share your thoughts on our Discord thread!

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

cmp-navigation/src/commonMain/kotlin/cmp/navigation/authenticated/AuthenticatedNavbarNavigationScreen.kt (1)

380-396: Hoist moreClientInfo callback to root scope, similar to onMoreInfoClicked fix.

The moreClientInfo callback (lines 382-387) also navigates to the DataTable route using the nested navController, which will cause the same IllegalArgumentException as the original onMoreInfoClicked issue. It should be hoisted to the root navigation scope and passed down, matching the fix pattern applied to onMoreInfoClicked.

feature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.kt (1)

362-368: Add loadMoreSavingsAccountInfo callback parameter to clientNavGraph for consistent navigation pattern.

The savingsDestination at line 365 uses navController::navigateToDataTable directly, inconsistent with how loanDestination handles the same scenario via the onMoreInfoClicked callback parameter. This creates a nested NavController issue: if the DataTable route is registered only in the root NavHost, navigation from the savings screen will fail with IllegalArgumentException.

Add a loadMoreSavingsAccountInfo: (String, Int) -> Unit callback parameter to clientNavGraph (matching the pattern used for onMoreInfoClicked in loanDestination), and pass it through to savingsDestination instead of the direct method reference.

🧹 Nitpick comments (3)

cmp-navigation/src/commonMain/kotlin/cmp/navigation/authenticated/AuthenticatedNavbarNavigationScreen.kt (2)

110-119: Parameter ordering: required parameter after optional one.

onMoreInfoClicked is a required parameter but is positioned after navController which has a default value. In Kotlin, required parameters should typically precede optional ones to allow callers to use positional arguments naturally. Also, there's inconsistent spacing (: vs :) with the colon.

Suggested reordering and spacing fix

 `@Composable`
 internal fun AuthenticatedNavbarNavigationScreen(
     navigateToDocumentScreen: (Int, String) -> Unit,
     onDrawerItemClick: (String) -> Unit,
+    onMoreInfoClicked: (String, Int) -> Unit,
     modifier: Modifier = Modifier,
     navController: NavHostController = rememberMifosNavController(
         name = "AuthenticatedNavbarScreen",
     ),
-    onMoreInfoClicked : (String, Int) -> Unit,
     viewModel: AuthenticatedNavbarNavigationViewModel = koinViewModel(),
 ) {

---

183-191: Same parameter ordering and spacing issue.

For consistency, onMoreInfoClicked should be moved before optional parameters and the spacing should be normalized to match other parameters.

Suggested fix

 `@Composable`
 internal fun AuthenticatedNavbarNavigationScreenContent(
     navController: NavHostController,
     onDrawerItemClick: (String) -> Unit,
     navigateToDocumentScreen: (Int, String) -> Unit,
+    onMoreInfoClicked: (String, Int) -> Unit,
+    onAction: (AuthenticatedNavBarAction) -> Unit,
     modifier: Modifier = Modifier,
     snackbarHostState: SnackbarHostState = remember { SnackbarHostState() },
-    onMoreInfoClicked : (String, Int) -> Unit,
-    onAction: (AuthenticatedNavBarAction) -> Unit,
 ) {

cmp-navigation/src/commonMain/kotlin/cmp/navigation/authenticated/AuthenticatedNavbarRoute.kt (1)

25-36: Minor: Missing space after comma in type signature.

The type signature (String,Int) at line 28 is missing a space after the comma. For consistency with Kotlin style conventions and other files in this PR, it should be (String, Int).

Suggested fix

 internal fun NavGraphBuilder.authenticatedNavbarGraph(
     onDrawerItemClick: (String) -> Unit,
     navigateToDocumentScreen: (Int, String) -> Unit,
-    onMoreInfoClicked : (String,Int) -> Unit,
+    onMoreInfoClicked: (String, Int) -> Unit,
 ) {

[Copilot]

Pull request overview

Fixes a crash (IllegalArgumentException) caused by navigating to the Data Table screen from within a nested (tab) NavController by hoisting the navigation event to the root graph.

Changes:

• Added an onMoreInfoClicked callback to clientNavGraph so Data Table navigation can be executed by the parent/root NavController.
• Threaded onMoreInfoClicked through authenticatedNavbarGraph and AuthenticatedNavbarNavigationScreen to bridge nested and root navigation graphs.
• Updated the loan destination wiring to use the hoisted callback instead of navController::navigateToDataTable.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File	Description
feature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.kt	Adds onMoreInfoClicked to the client nav graph and routes loan “More Info” through it.
cmp-navigation/src/commonMain/kotlin/cmp/navigation/authenticated/AuthenticatedNavigation.kt	Supplies the root navigateToDataTable callback into the authenticated navbar graph.
cmp-navigation/src/commonMain/kotlin/cmp/navigation/authenticated/AuthenticatedNavbarRoute.kt	Extends navbar graph API to accept and forward onMoreInfoClicked.
cmp-navigation/src/commonMain/kotlin/cmp/navigation/authenticated/AuthenticatedNavbarNavigationScreen.kt	Hoists and passes the callback down into the nested client navigation.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

cmp-navigation/src/commonMain/kotlin/cmp/navigation/authenticated/AuthenticatedNavbarNavigationScreen.kt (1)

380-388: ⚠️ Potential issue | 🟠 Major

Route DataTable navigation through the root controller.
moreClientInfo still calls navController.navigateToDataTable from the nested NavHost; if the DataTable destination lives only in the root graph (per the PR objective), this path can still crash. Consider delegating to onMoreInfoClicked (root nav) instead.

✅ Proposed fix

 clientNavGraph(
     navController = navController,
     moreClientInfo = { clientId ->
-        navController.navigateToDataTable(
-            Constants.DATA_TABLE_NAME_CLIENT,
-            clientId,
-        )
+        onMoreInfoClicked(
+            Constants.DATA_TABLE_NAME_CLIENT,
+            clientId,
+        )
     },
     onMoreInfoClicked = onMoreInfoClicked,
     activateClient = { clientId ->
         navController.navigateToActivateRoute(
             clientId,
             Constants.ACTIVATE_CLIENT,
         )
     },
     hasDatatables = navController::navigateDataTableList,
 )

[biplab1]

Looks good to me. This can be merged.