Fix reactive link between filter panel and modules

[m7pr]

THIS HAPPENS FOR Visualizations PANEL AS WELL

https://github.com/insightsengineering/teal.modules.clinical/assets/133694481/f5c2d5fc-aff3-41dd-aacb-3ad8f20bc71b

I am using https://genentech.shinyapps.io/nest_longitudinal_main/ (Demographic tab). This is the last state of the code that created the app the day I posted this comment https://github.com/insightsengineering/teal.gallery/blob/5816df5f41e7d2f1124807b5884d65f2315add31/longitudinal/app.R

When adding new filter or removing a filter the output gets rerendered even though no filters are really applied or removed as they are set to use all levels. Check out the video. This does not happen on a tab for View Data.

Code of Conduct

• [X] I agree to follow this project's Code of Conduct.

Contribution Guidelines

• [X] I agree to follow this project's Contribution Guidelines.

Security Policy

• [X] I agree to follow this project's Security Policy.

[lcd2yyz]

From UX perspective - No. If addition or removal of filter does not change the dimension of filtered data, then user would not want to see/wait for a re-render.

[m7pr]

@gogonzo do you think there is a specific reason such behavior occur? Do you recall if this behavior existed in teal since the beginning or is this something new that appeared recently on the filter-panel refactor?

[chlebowa]

I think this is not intended and unless I am mistaken fixing it would be a matter of changing one reactive dependency.

[gogonzo]

@m7pr @chlebowa yes, it is a real issue of filter panel. When filter is added and filtered data didn't change there should be no reactive trigger.

[lcd2yyz]

Convert to a bug issue?

[gogonzo]

Acceptance criteria:

• Applying filter which doesn't return any filter call (filter where everything is selected), shouldn't trigger teal_module calculations.
• influence should be as minimal as possible, bindCache of the data is least preffered solution
• close also this when done

[vedhav]

While debugging this reactivity issue I feel like there are additional redundant reactivity triggered even when the state is empty because by default observeEvent has ignoreInit = FALSEand ignoreNULL = TRUE. Even NULL cases can have false positives. For example (not sure if this is observed somewhere but it's just an example), we know that the teal_slices$slices being empty does not need to trigger anything but technically it's not NULL:

{
  "slices": [],
  "attributes": {
    "exclude_varnames" : {
      "ADTTE"          : ["STUDYID", "COUNTRY", "SITEID",...
    },
    "allow_add"        : true
  }
}

P.S IMO we have a lot of observers instead of reactives and we might not be taking advantage of Shiny's performance optimization when some other tab (module) is being changed for a certain part of the reactive chain.

[vedhav]

I suspect that this might be harder than just cutting off reactivity. I believe that the root cause of the issue is not because of unwanted things being observed but because the data has been changed to something and reassigned triggering invalidation by Shiny.

Shiny app showcasing the reactivity trigger

For example, run this small example to see that even though the value of my_reactive was not changed to a new value at the end of the observe expression, as soon as it is changed to a new value, Shiny invalidates it triggering all downstream reactive events hence the console prints "Rendering the text!" even though practically nothing has changed. Of course, the bindEvent here is useless but if you replace it with bindCache you'll see that it will start working fine.

library(shiny)

ui <- fluidPage(
    div(
        actionButton("click", "Click"),
        textOutput("text")
    )
)

server <- function(input, output, session) {
    observeEvent(input$click, {
        my_reactive("New Value")
        my_reactive("Old value")
    })
    my_reactive <- reactiveVal({
        "Old value"
    })
    output$text <- renderText({
        print("Rendering the text!")
        my_reactive()
    }) |>
        bindEvent(my_reactive()) # replace with bindCache to see it works fine
}

shinyApp(ui, server)

A simple Teal app to reproduce the reactivity issue

I'm just using the main branches of the teal packages. You'll see that the data hash is the same yet the histogram will rerender when you just add a filter. Again, this starts working fine once we replace the bindEvent with bindCache

devtools::load_all("teal.data")
devtools::load_all("teal.slice")
devtools::load_all("teal")

app <- init(
  data = teal_data(
    dataset("new_iris", iris),
    code = "new_iris <- iris"
  ),
  modules = module(
    "Iris Sepal.Length histogram",
    server = function(input, output, session, data) {
      output$hist <- renderPlot({
        print(
          paste0(
            "Rendering the histogram, data hash: ",
            digest::digest(data[["new_iris"]]()$Sepal.Length)
          )
        )
        hist(data[["new_iris"]]()$Sepal.Length)
      }) |>
        bindEvent(data[["new_iris"]]())  # replace with bindCache to see it works fine
    },
    ui = function(id, ...) {
      ns <- NS(id)
      plotOutput(ns("hist"))
    },
    datanames = "new_iris"
  )
)
shinyApp(app$ui, app$server)

[chlebowa]

I would rather we removed the cause of the behavior than hide the effects. Let me take another look.

[gogonzo]

Fixed in the last refactor