rememberRetained retains value of removed node

[vulpeszerda]

When calling a nested circuit screenB conditionally within screenA under the hierarchy of NavigableCircuitContent,
as in the following case:

val backStack = rememberSaveableBackStack(ScreenA)
navigator = rememberCircuitNavigator(backStack = backStack, onRootPop = {})
NavigableCircuitContent(navigator = navigator, backStack = backStack)

  private data object ScreenA : Screen {
    object State : CircuitUiState
  }

  private class ScreenAPresenter : Presenter<ScreenA.State> {
    @Composable
    override fun present(): ScreenA.State {
      return ScreenA.State
    }
  }

  @Composable
  private fun ScreenAUi(modifier: Modifier = Modifier) {
    Column(modifier) {
      val isChildVisible = remember { mutableStateOf(false) }
      Button(
        modifier = Modifier.testTag(TAG_BUTTON),
        onClick = { isChildVisible.value = !isChildVisible.value },
      ) {
        Text("toggle")
      }
      if (isChildVisible.value) {
        CircuitContent(screen = ScreenB)
      }
    }
  }

The rememberRetained called in the presenter of the nested circuit screenB retains its previous value even after screenB is removed and re-added. This behavior contrasts with rememberRetained called from the screenB's UI, which is reinitialized, suggesting this is a bug.

This issue only occurs when the presentWithLifecycle feature is set to true.

I have written a test code to reproduce this bug. Could you please take a look at it?
https://github.com/vulpeszerda/circuit/blob/navigable-circuit-content-test/circuit-foundation/src/jvmTest/kotlin/com/slack/circuit/foundation/NavigableCircuitContentRetainTest.kt

This bug only occurs when screenB is equal to the previously added screen instance.

[vulpeszerda]

I found and attached more test case in above link that rememberRetained works incorrectly even though presentWithLifecycle is false

[vulpeszerda]

I have been looking at implementation of rememberRetained for a long time, and it’s very confusing.

remember / rememberSaveable vs rememberRetained behave very differently, and this is not documented.

In the case of remember / rememberSaveable, the init block is executed each time it is added to a node based on the condition in the if-else block.

On the other hand, rememberRetained registers to the RetainedStateRegistry but does not call save when removed from the node. Therefore, when it is added back to the node later, there is no value to consume from the RetainedStateRegistry, so the init block is executed.

However, when rememberRetained is added and then removed from the node, unregister is not called from the RetainedStateRegistry. Thus, even after being removed from the node, the value is saved when save is called on the RetainedStateRegistry later. Subsequently, when rememberRetained is invoked again, the value is consumed, and the init block is not executed.

This behavior makes rememberRetained unpredictable. Therefore, the value of rememberRetained should be modified to behave like remember / rememberSaveable, where it is reset when added to and removed from the node.

[ZacSweers]

Hi, we'll take a look when we have time. Some feedback though - a lot of this has read like a list of complaints and then demands, I would encourage you to be a little more constructive and seeking to learn (perhaps using discussions to ask questions about the areas you are unsure about).

[vulpeszerda]

@ZacSweers
First of all, I sincerely apologize if my previous comment came across as too harsh or aggressive. I greatly appreciate and value the Circuit library—I even presented it at a recent developers' conference in Korea, showcasing my admiration and respect for it.

The reason my original message might have sounded critical is that I encountered this issue during the final stages of releasing a product that uses Circuit. After coding and analyzing the problem for over 20 hours, I was likely more sensitive than usual when writing the comment. If it caused any frustration, I hope you can kindly understand.

Regarding the issue, I don't have a deep enough understanding of the internal implementation of rememberRetained to make changes myself, so I initially reported it as a bug. However, I will review it further over the weekend to see if I can propose a possible solution.

Thank you for your understanding.

[vulpeszerda]

@ZacSweers
I’ve come up with a solution to address this issue. Except for needing to adjust a few test cases, it passes all existing tests, and I’ve modified rememberRetained to function similarly to remember / rememberSaveable.

I’ll add some more organized test cases and submit this as a PR 🙂

[ZacSweers]

Sounds good!