Could use some code review on a "wrapper" composable that I want to use in order to signify that a s...

# compose

[Colton Idle]

Could use some code review on a "wrapper" composable that I want to use in order to signify that a screen requires a logged in state.

Thoughts on this?

@Composable
fun RequireSignedInUser(
  navController: NavController,
  appStateHolder: AppStateHolder,
  content: @Composable () -> Unit
) {
  if (appStateHolder.isLoggedIn) { content() } else { LaunchedEffect(Unit) { navController.navigate(route = Screen.LoginScreen.route) }
  }
}

[Colton Idle]

Essentially I have this currently:

composable(Screen.Tab1.route) { Tab1Screen() }
composable(Screen.Tab2.route) { Tab2Screen() }
composable(Screen.Tab3.route) { Tab3Screen() }

but I want tab 1, 2, and 3 to all kick the user to the loginScreen if the user is logged out. So I created RequireSignedInUser and then use it as such

composable(Screen.Tab1.route) { RequireSignedInUser(navController, appStateHolder){Tab1Screen()} }
composable(Screen.Tab2.route) { RequireSignedInUser(navController, appStateHolder){Tab2Screen()} }
composable(Screen.Tab3.route) { RequireSignedInUser(navController, appStateHolder){Tab3Screen()} }

[Colton Idle]

The two things I'm "worried" most about is just my general usage of this wrapper for convenience. Maybe it's just kinda bad to follow this pattern?

Second thing I'm worried about is the LaucnhedEffect(Unit) inside of an if statement. Kinda feels smelly?

[tad]

I would perform the signed-in-user logic in the layer that is hosting these tabs, then you don't need the side effect in each destination composable

[Zach Klippenstein (he/him) [MOD]]

Better to just compose the login screen directly instead of trampolining a navigation call.

[Colton Idle]

I def feel this way as well, but my team is going with the direction that the arch events docs state. Which basically uses nav calls.

[Colton Idle]

i.e.

[Tash]

is AppStateHolder stable/immutable? i wonder if there’s overhead in adding the wrapper for each destination. Agree with Tad, the login check should happen outside of the nav graph (i.e you dont see that nav graph unless you are logged in)

[Tash]

Curious, what did u end up going with?

[Colton Idle]

At the moment I'm going for basically what the docs show. So the user technically always sees the nav graph and the check is done per destination.